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

[WICKET-7069] Refactor assertTrue(equals()) using assertEquals #601

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

Taher-Ghaleb
Copy link
Contributor

I am working on research that investigates test smell refactoring in which we identify alternative implementations of test cases, study how commonly used these refactorings are, and assess how acceptable they are in practice.

The first smell is when inappropriate assertions are used, while there exist better alternatives. For example, in throws Exception using assertThrows, I refactored assertTrue(img.getName().equals("img")); using assertEquals(img.getName(), "img");.

The second smell is when exception handling can alternatively be implemented using assertion rather than annotation. For example, in ApplicationSettingsTest.java, I used assertThrows(Exception.class, () -> {...}); instead of throws Exception.

While there could be several cases like this, we aim in this pull request to get your feedback on these particular test smells and their refactorings. Thanks in advance for your input.

@solomax
Copy link
Contributor

solomax commented Aug 24, 2023

Hello @Taher-Ghaleb

please revert your commit labeled [Refactor throws Exception using assertThrows](https://github.com/apache/wicket/pull/601/commits/d667bafbb8081b87fc6dc764f7fc895b066b96d7)

It breaks the build, because:

If tests is defined as follows:

@Test
	void testExceptionOnMissingResourceDefaultValue() throws Exception

It doesn't mean it expects the Exception to be thrown
It only states some method might throw Exception so we should add try-catch OR throws Exception to satisfy compiler :)

@Taher-Ghaleb
Copy link
Contributor Author

Thanks @solomax for your feedback.

I must have misunderstood the actual objective of these cases. I will revert that commit shortly.

In our research, we have actually encountered test cases where developers refactor exception handling operations using the assertThrows assertion (e.g., instead of using @Test(expected)). Therefore, we wanted to understand whether such refactoring is commonly acceptable in practice by developers and whether there are certain cases where it is not preferred by submitting such pull requests, but I mistakenly chose test cases with different behavior. Thanks.

@solomax
Copy link
Contributor

solomax commented Aug 24, 2023

where developers refactor exception handling operations using the assertThrows assertion (e.g., instead of using @test(expected))

AFAIK @Test(expected) is JUnit-4 way of checking exception was thrown, while assertThrows is JUnit-5 way :)))

for ex. see here: https://www.baeldung.com/junit-5-migration#1-annotations

@Taher-Ghaleb
Copy link
Contributor Author

Should I change the title and description of the pull request to reflect the eventual changes performed?

@solomax
Copy link
Contributor

solomax commented Aug 24, 2023

Should I change the title and description of the pull request to reflect the eventual changes performed?

Yes, please :))
Additionally JIRA issue need to be created here https://issues.apache.org/jira/projects/WICKET

I can take care of both steps :)

@solomax solomax changed the title Refactor assertTrue(equals()) using assertEquals & throws Exception using assertThrows Refactor assertTrue(equals()) using assertEquals Aug 24, 2023
@solomax solomax changed the title Refactor assertTrue(equals()) using assertEquals [WICKET-7069] Refactor assertTrue(equals()) using assertEquals Aug 24, 2023
@solomax solomax merged commit 3928c38 into apache:master Aug 24, 2023
1 check passed
solomax pushed a commit that referenced this pull request Aug 24, 2023
)

* Refactor assertTrue(equals()) using assertEquals

* Refactor throws Exception using assertThrows

* Revert "Refactor throws Exception using assertThrows"

This reverts commit d667baf.

* Fix assertEquals parameters as (expected, actual)

* Fix test case failures by converting tokens into strings
@solomax
Copy link
Contributor

solomax commented Aug 24, 2023

Thanks for the contribution @Taher-Ghaleb !

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.

3 participants