Enable jtcop #3113

c71n93 · 2024-04-19T10:46:39Z

PR-Codex overview

This PR updates test assertion messages to use a constant AtCompositeTest.TO_ADD_MESSAGE.

Detailed summary

Updated test assertion messages to use AtCompositeTest.TO_ADD_MESSAGE constant
Replaced hardcoded assertion messages with a constant in multiple test files

The following files were skipped due to too many changes: eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyRemoteTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/LatexMojoTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/CleanMojoTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/hash/ChNarrowTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/dependencies/DcsDepgraphTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOseqTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/rust/ModuleTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/rust/PrimeModuleTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/rust/NativeTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOerrorTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/hash/ChCachedTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOstringTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/MarkMojoTest.java, eo-runtime/src/test/java/org/eolang/PhiTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyEmptyTest.java, eo-parser/src/test/java/org/eolang/parser/ObjectsTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/footprint/CacheVersionTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/tojos/ForeignTojosTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/dependencies/DcsWithRuntimeTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/util/RelTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/CopyMojoTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/SodgMojoTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOintTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/RegisterMojoTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/util/FileHashTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/hash/ChTextTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOio/EOstdoutTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyCachingTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/hash/ChRemoteTest.java, eo-runtime/src/test/java/org/eolang/DataizedTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/rust/NamesTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyHomeTest.java, eo-runtime/src/test/java/org/eolang/AtCompositeTest.java, eo-runtime/src/test/java/org/eolang/PhLoggedTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOtryTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyFilesystemTest.java, eo-runtime/src/test/java/org/eolang/DataTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOtupleEOatTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/ParseMojoTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/ObjectsIndexTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/PullMojoTest.java, eo-runtime/src/test/java/org/eolang/PhMethodTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/dependencies/DcsEachWithoutTransitiveTest.java, eo-runtime/src/test/java/org/eolang/PhWithTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/footprint/FtDefaultTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyIndexedTest.java, eo-parser/src/test/java/org/eolang/parser/StUnhexTest.java, eo-runtime/src/test/java/org/eolang/AtLoggedTest.java, eo-runtime/src/test/java/EOorg/EOeolang/CagesTest.java, eo-parser/src/test/java/org/eolang/parser/EoIndentLexerTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyFallbackSwapTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/footprint/FtCachedTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/tojos/TranspiledTojosTest.java, eo-runtime/src/test/java/org/eolang/UniverseDefaultTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/BinarizeMojoTest.java, eo-runtime/src/test/java/org/eolang/MainTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/objectionary/OyFallbackTest.java, eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/UnplaceMojoTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOcageTest.java, eo-runtime/src/test/java/org/eolang/BytesOfTest.java, eo-runtime/src/test/java/org/eolang/PhPackageTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/OptimizeMojoTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/util/HmBaseTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/BinarizeParseTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOio/EOstdinTest.java, eo-runtime/src/test/java/EOorg/EOeolang/EOmemoryTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/ResolveMojoTest.java, eo-runtime/src/test/java/EOorg/EOeolang/HeapsTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/TranspileMojoTest.java, eo-runtime/src/test/java/org/eolang/PhDefaultTest.java, eo-maven-plugin/src/test/java/org/eolang/maven/PlaceMojoTest.java

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

c71n93 · 2024-04-19T11:24:19Z

@volodya-lombrozo could you check this one, please?

volodya-lombrozo

@c71n93 Well done! Just a few suggestions.

volodya-lombrozo · 2024-04-19T11:35:56Z

eo-maven-plugin/src/test/java/org/eolang/maven/BinarizeMojoTest.java

@@ -71,7 +71,8 @@ void binarizesWithoutErrors(@TempDir final Path temp) throws Exception {
                .withProgram(BinarizeMojoTest.SRC.resolve("twice-rust.eo"));
        }
        Assertions.assertDoesNotThrow(
-            () -> maven.execute(new FakeMaven.Binarize())
+            () -> maven.execute(new FakeMaven.Binarize()),
+            "TO ADD ASSERTION MESSAGE"


@c71n93 I would suggest to create a single static field and to use it everywhere instead.

Something like is as follows:

/** * Don't forget to add 'todo' puzzle to remove this field later */ public static final String TODO_MESSAGE = "TO ADD ASSERTION MESSAGE";

(In this case you might need to suppress some checkstyle and PMD warnings)

@volodya-lombrozo I think it is better to keep repeating message where it is allowed. The advantages of this is that you will not need to think about deleting unused field when meaningful messages will be added. And I don't see any disadvantages.

We already have a check for this, and this check prohibits more than 5 repetitions of the same values (and in such cases I did as you suggested).

@c71n93, I haven't clearly understood the "advantages" you described. To me, they actually seem like disadvantages. Anyway.

Please take another look at this code and pay attention to the comment. I suggested this change intentionally.

Why we should opt for only one field instead of many different fields and repeated "TO ADD ASSERTION MESSAGE" constants:

You have only one place where you define this message.

You have only one place where you can add a todo puzzle for further changes.

It's the only place that has a bad smell. Otherwise you infecting the codebase by suboptimal code in many places.

Otherwise, if you leave it as it is:

You need to add a separate puzzle for each field.

If you leave the "TO ADD ASSERTION MESSAGE" constants (that aren't attached to any field and a puzzle), we will just forget to replace them with meaningful messages.

You have to deal with the checkstyle check:

this check prohibits more than 5 repetitions of the same values

@c71n93 Moreover, in the future versions of jtcop we will forbid the usage of static literals. So, later we will need to remove all of this static literals.

@c71n93 Maybe we will add three similar constants for each submodule:

// todo: remove this field from 'eo-runtime' public static final String TODO_MESSAGE = "TO ADD ASSERTION MESSAGE"; // todo: remove this field from 'eo-parser' public static final String TODO_MESSAGE = "TO ADD ASSERTION MESSAGE"; // todo: remove this field from 'eo-maven-plugin' public static final String TODO_MESSAGE = "TO ADD ASSERTION MESSAGE";

In this case we will have 3 separate todos which will be much easier to fix and we don't need to think how to share the same field across modules. What do you think?

Yes, it is some sort of duplication, but it's simple, as for me.

add such public static fields with todo to any test class

Not exactly, I suggest to leave only one field and use it across all of the other classes.

@volodya-lombrozo Ok, understood. I free to choose any class in every submodule?

@c71n93 Yes, it doesn't matter which class you will choose, since we will remove it later.

volodya-lombrozo · 2024-04-19T11:38:04Z

eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java

@@ -180,7 +180,8 @@ void parsesSuccessfully(final String code) {
            new InputOf(code)
        );
        Assertions.assertDoesNotThrow(
-            syntax::parsed
+            syntax::parsed,
+            EoSyntaxTest.EMPTY_MSG


@c71n93 Maybe it's better to use the same constant value "TO ADD ASSERTION MESSAGE" here? Otherwise we might forget to add a meaningful message later.

@volodya-lombrozo I think we can rename EMPTY_MSG to TODO_MESSAGE as you suggested above. We can't use value "TO ADD ASSERTION MESSAGE" because checkstyle prohibits many repetitions of the same value.

@volodya-lombrozo or maybe TO_ADD_MESSAGE would be better?

@c71n93 I'm not sure that we can use 3 words in constant identifiers. Most probably, checkstyle will forbid TO_ADD_MESSAGE constant, but you can try.

maxonfjvipon · 2024-04-19T12:34:58Z

@c71n93 @volodya-lombrozo I would suggest also to add todo to rewrite all strings "TO_ADD_MESSAGE" with really meaningful messages in all tests

c71n93 · 2024-04-19T12:43:36Z

@maxonfjvipon I agree, but where to add this todo? I think it should be only one todo, because If we add it to every test class it would be too many issues.

maxonfjvipon · 2024-04-19T12:45:18Z

@c71n93 I think it does not really matter, It can be any test

volodya-lombrozo · 2024-04-19T13:09:47Z

eo-maven-plugin/src/test/java/org/eolang/maven/BinarizeMojoTest.java

@@ -71,7 +71,8 @@ void binarizesWithoutErrors(@TempDir final Path temp) throws Exception {
                .withProgram(BinarizeMojoTest.SRC.resolve("twice-rust.eo"));
        }
        Assertions.assertDoesNotThrow(
-            () -> maven.execute(new FakeMaven.Binarize())
+            () -> maven.execute(new FakeMaven.Binarize()),
+            "TO ADD ASSERTION MESSAGE"


@c71n93, I haven't clearly understood the "advantages" you described. To me, they actually seem like disadvantages. Anyway.

Please take another look at this code and pay attention to the comment. I suggested this change intentionally.

Why we should opt for only one field instead of many different fields and repeated "TO ADD ASSERTION MESSAGE" constants:

You have only one place where you define this message.

You have only one place where you can add a todo puzzle for further changes.

It's the only place that has a bad smell. Otherwise you infecting the codebase by suboptimal code in many places.

Otherwise, if you leave it as it is:

You need to add a separate puzzle for each field.

If you leave the "TO ADD ASSERTION MESSAGE" constants (that aren't attached to any field and a puzzle), we will just forget to replace them with meaningful messages.

You have to deal with the checkstyle check:

this check prohibits more than 5 repetitions of the same values

volodya-lombrozo

@c71n93 Can you move this description directly to the field javadoc, not to a class:

 /** @todo #2297:60m Replace all appearances of {@link AtCompositeTest#TO_ADD_MESSAGE} field in
  *  eo-runtime with meaningful assert messages. Don't forget to remove
  *  {@link AtCompositeTest#TO_ADD_MESSAGE} field and remove public modifier from this class if no
   longer need. */
  public static final String TO_ADD_MESSAGE = AtCompositeTest.TO_ADD_MESSAGE;

c71n93 · 2024-04-19T15:53:35Z

@volodya-lombrozo could you check again please?

volodya-lombrozo

@c71n93 Well done! Looks good to me. Thank you for the contribution.

c71n93 · 2024-04-21T14:59:41Z

@yegor256 could you check this one, please?

c71n93 · 2024-04-22T09:27:38Z

@yegor256 could you check this one, please?

yegor256 · 2024-04-23T02:37:10Z

@rultor merge

rultor · 2024-04-23T02:39:59Z

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2024-04-23T02:59:01Z

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 18min)

c71n93 added 5 commits April 16, 2024 13:33

objectionary#2297 jtcop version up and assertThrows message resolve

57fb112

objectionary#2297 add stub assertion messages for eo-runtime

11c1402

objectionary#2297 jtcop version up

7153999

Merge branch 'master' into 2297-enable-jtcop

96f9f5c

rerun CI

5b63353

volodya-lombrozo suggested changes Apr 19, 2024

View reviewed changes

objectionary#2297 rename empty message fields

21be3ca

objectionary#2997 add todo for replacing empty messages

5228a97

volodya-lombrozo suggested changes Apr 19, 2024

View reviewed changes

c71n93 added 2 commits April 19, 2024 17:48

objectionary#2297 resolved empty message duplication for eo-maven-plugin

7e465e1

objectionary#2297 resolve empty message duplication for eo-runtime

e89c483

volodya-lombrozo reviewed Apr 19, 2024

View reviewed changes

c71n93 added 4 commits April 19, 2024 18:09

objectionary#2297 resolve empty message duplication for eo-parser

4c24b95

objectionary#2297 fix todo

9008a41

Merge branch 'master' into 2297-enable-jtcop

f0328e2

objectionary#2297 fix checkstyle and todo

b477065

c71n93 force-pushed the 2297-enable-jtcop branch from b060827 to b477065 Compare April 19, 2024 15:29

volodya-lombrozo approved these changes Apr 21, 2024

View reviewed changes

rultor merged commit 191151f into objectionary:master Apr 23, 2024
17 checks passed

c71n93 deleted the 2297-enable-jtcop branch May 6, 2024 09:37

c71n93 mentioned this pull request May 14, 2024

#3160 Enable PMD for eo-runime #3171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable jtcop #3113

Enable jtcop #3113

c71n93 commented Apr 19, 2024 •

edited by pr-codex bot

Loading

c71n93 commented Apr 19, 2024

volodya-lombrozo left a comment

volodya-lombrozo Apr 19, 2024

volodya-lombrozo Apr 19, 2024

c71n93 Apr 19, 2024

volodya-lombrozo Apr 19, 2024 •

edited

Loading

volodya-lombrozo Apr 19, 2024

volodya-lombrozo Apr 19, 2024

volodya-lombrozo Apr 19, 2024 •

edited

Loading

volodya-lombrozo Apr 19, 2024

c71n93 Apr 19, 2024

volodya-lombrozo Apr 19, 2024

volodya-lombrozo Apr 19, 2024

c71n93 Apr 19, 2024

c71n93 Apr 19, 2024 •

edited

Loading

volodya-lombrozo Apr 19, 2024 •

edited

Loading

maxonfjvipon commented Apr 19, 2024

c71n93 commented Apr 19, 2024

maxonfjvipon commented Apr 19, 2024

volodya-lombrozo Apr 19, 2024 •

edited

Loading

volodya-lombrozo left a comment

c71n93 commented Apr 19, 2024

volodya-lombrozo left a comment

c71n93 commented Apr 21, 2024

c71n93 commented Apr 22, 2024

yegor256 commented Apr 23, 2024

rultor commented Apr 23, 2024

rultor commented Apr 23, 2024

Enable jtcop #3113

Enable jtcop #3113

Conversation

c71n93 commented Apr 19, 2024 • edited by pr-codex bot Loading

PR-Codex overview

Detailed summary

c71n93 commented Apr 19, 2024

volodya-lombrozo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodya-lombrozo Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodya-lombrozo Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c71n93 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

volodya-lombrozo Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

maxonfjvipon commented Apr 19, 2024

c71n93 commented Apr 19, 2024

maxonfjvipon commented Apr 19, 2024

volodya-lombrozo Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

volodya-lombrozo left a comment

Choose a reason for hiding this comment

c71n93 commented Apr 19, 2024

volodya-lombrozo left a comment

Choose a reason for hiding this comment

c71n93 commented Apr 21, 2024

c71n93 commented Apr 22, 2024

yegor256 commented Apr 23, 2024

rultor commented Apr 23, 2024

rultor commented Apr 23, 2024

c71n93 commented Apr 19, 2024 •

edited by pr-codex bot

Loading

volodya-lombrozo Apr 19, 2024 •

edited

Loading

volodya-lombrozo Apr 19, 2024 •

edited

Loading

c71n93 Apr 19, 2024 •

edited

Loading

volodya-lombrozo Apr 19, 2024 •

edited

Loading

volodya-lombrozo Apr 19, 2024 •

edited

Loading