Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup XML for SelectValues transform #4361

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

xdelox
Copy link

@xdelox xdelox commented Sep 25, 2024

Fixes #1929


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Run mvn clean install apache-rat:check to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If you have a group of commits related to the same change, please squash your commits into one and force push your branch using git rebase -i.
  • Mention the appropriate issue in your description (for example: addresses #123), if applicable.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@@ -29,4 +29,12 @@
<packaging>jar</packaging>
<name>Hop Plugins Transforms Select Values</name>

<dependencies>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this for writing assertions in a comfortable and readable way. Ideally we could introduce it in parent project. Will send a proposal.

selectValuesMeta
.getSelectOption()
.setSelectFields(SelectValueMetaTestFactory.getSelectFields("v", "v2"));
setup(selectValuesMeta);
}

@Test
public void test() throws Exception {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't pass. I can't actually understand how it's building up the meta object, because apparently, it was empty, previously.
Now, with the new structure I get a IndexOutOfBoundException, which I can't fix without understanding what's the subject under test and how I can put it together.

@@ -40,9 +40,13 @@
public class SelectValuesMetaTest {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should be ok. I've removed a few tests that wouldn't make any sense with the current structure.
Well, the remaining ones maybe neither, but less than the evicted ones. 😄

However the focus of this class was already the SelectFields tab, while the Remove and ChangeMetadata part was skipped over completely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether I should restore this, as I have no background of the underlying PDI issues...

@@ -145,6 +137,6 @@ private void executeAndCheck(String locale, String expectedWeekNumber) throws Ex

List<Object[]> execute = PipelineTestingUtil.execute(transform, 1, true);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is returning a formatted date, rather than the expected number of week. Probably the defaultFormatLocale is not working as expected. Heads up are more than welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Cleanup XML of transform Select Values
1 participant