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.

Sign up for GitHub

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

Jump to bottom

assertEquals(Set,Set) now ignores ordering as it did before, fixes #2643 #2648

Merged
merged 1 commit into from
Oct 28, 2021
Merged

assertEquals(Set,Set) now ignores ordering as it did before, fixes #2643 #2648

merged 1 commit into from
Oct 28, 2021

Conversation

Elisedlund-ericsson
Copy link
Contributor

see #2643

the https://github.com/cbeust/testng/blob/master/CONTRIBUTING.md is linked from the readme.md but does not exist from what I can see, (so excuse me if I didn't follow the correct WoW)

@Elisedlund-ericsson Elisedlund-ericsson mentioned this pull request Oct 5, 2021
Closed
@juherr
Copy link
Member

juherr commented Oct 7, 2021

Thank for the fix. I'm always feeling nervous when we change a test and even more when the test is linked to another issue.

We will review the changes carefully asap.

@Elisedlund-ericsson
Copy link
Contributor Author

I don't see how these CI failures are related to my pullrequest, it looks like unstable test more or less.

@juherr
Copy link
Member

juherr commented Oct 11, 2021

I don't see how these CI failures are related to my pullrequest, it looks like unstable test more or less.

Don't worry. Tests are failing with some specific environments.

@martinaldrin
Copy link

I wonder when do you think this PR could be submitted?
I also wonder what is the plan for the official release of TestNg 7.5

@juherr
Copy link
Member

juherr commented Oct 24, 2021

@krmahadevan Could you have a look at it? I think it will make a regression for #2540 Could you confirm?

@krmahadevan
Copy link
Member

@krmahadevan Could you have a look at it? I think it will make a regression for #2540 Could you confirm?

@juherr - The fix for #2540 involved changing things in assertions that deal with collections, wherein the assertion was to take into account ordering (to adhere to the documentation). The changes in this PR is hovering around assertions that deal with sets equality wherein we are not supposed to check for ordering but just fall back to the equality evaluation in the respective class. I dont see any regression. Also I dont see any test failing as well.

@juherr
Copy link
Member

juherr commented Oct 28, 2021

Ok. I understand where was my mistake: I didn't see the cast into Collection.

juherr
juherr requested changes Oct 28, 2021
View reviewed changes
Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

Update tests in order to check both GITHUB-2540 and GITHUB-2643.

testng-asserts/src/test/java/test/asserttests/AssertTest.java Show resolved Hide resolved
martinaldrin
martinaldrin reviewed Oct 28, 2021
View reviewed changes
CHANGES.txt Outdated
@@ -1,4 +1,5 @@
Current
Fixed: GITHUB-2643: assertEquals(Set,Set) now ignores ordering as it did before.
Copy link

Choose a reason for hiding this comment

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

add (Elis Edlund)

@martinaldrin
Copy link

@Elisedlund-ericsson I think that you need to squash your commits.

juherr
juherr requested changes Oct 28, 2021
View reviewed changes
testng-asserts/src/test/java/test/asserttests/AssertTest.java Outdated
@@ -400,24 +400,42 @@ public void checkSetEquals() {
assertEquals(set1, set2);
}

@Test(description = "GITHUB-2540")
public void checkSetNotEquals() {
@Test(description = "GITHUB-2540 and GITHUB-2643", expectedExceptions = AssertionError.class)
Copy link
Member

Choose a reason for hiding this comment

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

Remove reference to GITHUB-2540

testng-asserts/src/test/java/test/asserttests/AssertTest.java Show resolved Hide resolved
@juherr juherr merged commit d615583 into testng-team:master Oct 28, 2021
@testng-team testng-team deleted a comment from pancht Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinaldrin martinaldrin martinaldrin left review comments

@juherr juherr juherr requested changes

@krmahadevan krmahadevan krmahadevan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Elisedlund-ericsson @juherr @martinaldrin @krmahadevan