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

Task #3132: Removing AtCompositeTest.TO_ADD_MESSAGE #3187

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

Conversation

ZacParize
Copy link

@ZacParize ZacParize commented May 14, 2024

TODO:
replace all appearances of AtCompositeTest.TO_ADD_MESSAGE field in eo-runtime with meaningful assert messages
and don't forget to remove public modifier from this class if no longer need.

DONE:

  1. Moved from AtCompositeTest.TO_ADD_MESSAGE to AtCompositeTest.FAILED_ASSERT_MESSAGE_SUPPLIER in tests because of more reasonable assert message.

  2. Didn't created own FAILED_ASSERT_MESSAGE_SUPPLIER in every test class because of boilerplate.
    It doesn't make any sense.

  3. Left public modifier for AtCompositeTest because it reuses in other tests.


PR-Codex overview

This PR updates test assertions in multiple test files to use AtCompositeTest.FAILED_ASSERT_MESSAGE_SUPPLIER.get() instead of AtCompositeTest.TO_ADD_MESSAGE.

Detailed summary

  • Replaced AtCompositeTest.TO_ADD_MESSAGE with AtCompositeTest.FAILED_ASSERT_MESSAGE_SUPPLIER.get() in multiple test files for improved test assertion messages.

The following files were skipped due to too many changes: eo-runtime/src/test/java/EOorg/EOeolang/EOtryTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOio/EOstdoutTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOtupleEOatTest.java, eo-runtime/src/test/java/org/eolang/PhMethodTest.java, eo-runtime/src/test/java/org/eolang/PhLoggedTest.java, eo-runtime/src/test/java/org/eolang/DataTest.java, eo-runtime/src/test/java/EOorg/EOeolang/CagesTest.java, eo-runtime/src/test/java/org/eolang/PhWithTest.java, eo-runtime/src/test/java/org/eolang/AtCompositeTest.java, eo-runtime/src/test/java/org/eolang/AtLoggedTest.java, eo-runtime/src/test/java/org/eolang/MainTest.java, eo-runtime/src/test/java/org/eolang/UniverseDefaultTest.java, eo-runtime/src/test/java/org/eolang/PhPackageTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOcageTest.java, eo-runtime/src/test/java/org/eolang/BytesOfTest.java, eo-runtime/src/test/java/EOorg/EOeolang/HeapsTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOio/EOstdinTest.java, eo-runtime/src/test/java/org/eolang/PhDefaultTest.java

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

…positeTest.FAILED_ASSERT_MESSAGE_SUPPLIER in tests
@maxonfjvipon
Copy link
Member

@ZacParize thanks for your pull request. To start review we need all GHA jobs are green. You have only 5 qulice violations which is easy to resolve. Tag me if you need help

@ZacParize
Copy link
Author

ZacParize commented May 15, 2024

@maxonfjvipon Thank you for your answer. I've seen logs, there are issues connected with stands (EOF, unable to delete directory and so on). Please, give me a clue what i have to do.

@maxonfjvipon
Copy link
Member

maxonfjvipon commented May 15, 2024

@ZacParize these are the violations you need to fix:

[INFO] --- qulice-maven-plugin:0.23.0:check (jcabi-qulice-check) @ eo-runtime ---
[INFO] Checkstyle: eo-runtime/src/test/java/org/eolang/AtCompositeTest.java[29]: Empty line or comment between imports is not allowed (ImportCohesionCheck)
[INFO] Checkstyle: eo-runtime/src/test/java/org/eolang/AtCompositeTest.java[30]: Extra separation in import group before 'org.hamcrest.MatcherAssert' (ImportOrderCheck)
[INFO] Checkstyle: eo-runtime/src/test/java/org/eolang/AtCompositeTest.java[49]: Indentation (12) must be same or less than previous line (4), or bigger by exactly 4 (CascadeIndentationCheck)
[INFO] Checkstyle: eo-runtime/src/test/java/org/eolang/AtCompositeTest.java[50]: Indentation (20) must be same or less than previous line (12), or bigger by exactly 4 (CascadeIndentationCheck)
[INFO] Checkstyle: eo-runtime/src/test/java/org/eolang/AtCompositeTest.java[51]: Concatenation of string literals prohibited (StringLiteralsConcatenationCheck)

@ZacParize
Copy link
Author

@maxonfjvipon Looks like all checks have completed.

@maxonfjvipon
Copy link
Member

@ZacParize thanks again for your contribution, but this ticket was about to replace static string with some Meaningful message which would help a programmer to understand what exactly is wrong. You can read more here about it.
Here we have just a name of the failed test class test which is not really informative and may be seen in the console when tests fails.

@ZacParize
Copy link
Author

@maxonfjvipon Please, check out my last commit. Have i choose right test message format for completing the task?
It does not have any sense to move to new format all tests so I decided to redo only this set of tests. If it is ok, i move on.

Copy link
Member

@maxonfjvipon maxonfjvipon left a comment

Choose a reason for hiding this comment

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

@ZacParize thanks, that's better but there are a few things that can be improved

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.

None yet

2 participants