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

Migrate from TestNG to JUnit5 #1263

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

apupier
Copy link
Contributor

@apupier apupier commented Apr 19, 2024

  • all compilation issues fixed
  • remains few failing tests

Before you submit a pull request please acknowledge the following:

  • You have signed an Eclipse Contributor Agreement and are committing using the same email address
  • Your code contains any tests relevant to the problem you are solving
  • All new and existing tests passed
  • Code follows the style guidelines and Checkstyle passes

See CONTRIBUTING for more information.

@eclipse-milo-bot
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@eclipse-milo-bot
Copy link
Contributor

Can one of the admins verify this patch?

@apupier apupier force-pushed the migrateToJunitTest branch 5 times, most recently from dd2f417 to 8db1853 Compare April 19, 2024 13:54
Signed-off-by: Aurélien Pupier <[email protected]>
@apupier apupier marked this pull request as ready for review April 19, 2024 13:58
@apupier
Copy link
Contributor Author

apupier commented Apr 19, 2024

Please note that I'm on PTO next week. Feel free to update/modify if needed

@kevinherron
Copy link
Contributor

Thanks for picking this one up. I'll try to take a closer look at this today or over the weekend.

On first glance it looks mostly good, except I think you missed the most annoying thing about the migration, which is that JUnit and TestNG assertions swap the position of the "expected" and "actual" parameters 😿

@apupier
Copy link
Contributor Author

apupier commented Apr 19, 2024

which is that JUnit and TestNG assertions swap the position of the "expected" and "actual" parameters

argh and there is 1335 matches just for assertEquals

image

and after a quick searchI have not found a reliable way to switch them automatically

@apupier
Copy link
Contributor Author

apupier commented Apr 19, 2024

Provided as a separate commit to more easily revert in case there is a problem for assertEquals using:

image

@kevinherron
Copy link
Contributor

Careful, I think you might be hitting existing JUnit tests and swapping the positions of those assertions.

@apupier
Copy link
Contributor Author

apupier commented Apr 19, 2024

Careful, I think you might be hitting existing JUnit tests and swapping the positions of those assertions.

hum, I didn't noticed that there were existing Junit tests. Effectively I hit them.
I need to stop for today and then I have a week of PTO.
Feel free to pick the commits you are interested in, or to start from scratch.

@apupier
Copy link
Contributor Author

apupier commented Apr 19, 2024

and thanks for the quick feedback!

@kevinherron
Copy link
Contributor

Enjoy your PTO, thanks for the contribution.

@kevinherron
Copy link
Contributor

IntelliJ allows you to specify the scope of a Find (and Replace) action, and it looks like local changes is one of those scopes. I might be able to get something working using that... I'll play with it a bit.

@protogenes
Copy link

When talking about IntelliJ there is Structural Replace which allows you to find all method calls to the testng Assertion class and replace them with switched first and second parameters while leaving the remaining arguments unchanged (e.g. message)
I would go with this, is has a steeper learning curve but is worth it.

@kevinherron
Copy link
Contributor

@protogenes that's another good solution, though it would have to be applied as the first step, before all the package migration from TestNG to JUnit happens.

@VSSW-GTN
Copy link

Would it be worth it to have a look at AssertJ in the context of the migration towards JUnit5? The fluent syntax helps with making the actual assertions a bit clearer to read and understand if there are multiple conditions being checked on a given result, e.g.:

assertEquals(5, nodes.size());
assertTrue(nodes.stream().anyMatch(n -> n.getNodeId().equals(Identifiers.Server_ServerArray)));
assertTrue(nodes.stream().anyMatch(n -> n.getNodeId().equals(Identifiers.Server_NamespaceArray)));
assertTrue(nodes.stream().anyMatch(n -> n.getNodeId().equals(Identifiers.Server_ServiceLevel)));
assertTrue(nodes.stream().anyMatch(n -> n.getNodeId().equals(Identifiers.Server_Auditing)));
assertTrue(nodes.stream().anyMatch(n -> n.getNodeId().equals(Identifiers.Server_EstimatedReturnTime)));

would become

assertThat(nodes)
  .extracting(UaNode::getNodeId)
  .containsExactlyInAnyOrder(
    Identifiers.Server_ServerArray,
    Identifiers.Server_NamespaceArray,
    Identifiers.Server_ServiceLevel,
    Identifiers.Server_Auditing,
    Identifiers.Server_EstimatedReturnTime);

If this is something that might be of interest, I could start working on a PR to preview how this would look like if applied across the board.

@kevinherron
Copy link
Contributor

@VSSW-GTN thanks but I'm not interested in adding another library. I've never been convinced that assertion "DSLs" like this end up being worth it in the end. Regular assertions aren't difficult to read and don't come with the overhead of needing to understand the assertion language of whatever library you're using. Maybe if the tests were complex enough it would start to make sense.

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

Successfully merging this pull request may close these issues.

5 participants