BAEL-7006 Create example code for environment variables article #14817
Conversation
b973cec to
16fac85
Compare
16fac85 to
407fbd7
Compare
| import java.util.Map; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| // This test can only work well in Java 15 and below |
There was a problem hiding this comment.
Which of these tests fails on Java 16+? How about using something like this on those test methods:
@EnabledForJreRange(min = JRE.JAVA_8, max = JRE.JAVA_15) instead of the comment
There was a problem hiding this comment.
Nice. I like it.
There was a problem hiding this comment.
Hmm, I am confused now, since CI runs this module on Java 17, the build should have failed previously for one of these tests but it did not. Can you please investigate running this test on Java 17 and understand how it is passing when you expected it to fail please?
There was a problem hiding this comment.
And also can you please clarify which of the two tests you expected to fail on Java 16+?
| assertThat(reader.whatOs()) | ||
| .isEqualTo("MacDowsNix"); |
There was a problem hiding this comment.
I would usually expect to see single element assertions on a single line, even when using AssertJ - but I guess this is a matter of style so if you feel strongly about it, feel free to leave it
There was a problem hiding this comment.
I've put it in the same shape as the example code in the article. At this size, I'm not sure whether I'd split the line in real life. I quite like fluent code to line break at the '.'
There was a problem hiding this comment.
Prior to your changes the pom already had in the surefire plugin definition:
--add-opens java.base/java.util=ALL-UNNAMED
--add-opens java.base/java.lang=ALL-UNNAMED
Since the article mentions sensitivity to particular Java versions, I am wondering if it would be better if we had a clean surefire run with default arguments, instead of these additional arguments (which may or may not have any effect on the tests, I did not investigate further).
There was a problem hiding this comment.
To be fair, I don't expect us to simulate a failure in these tests. Our builds are all at much lower versions of Java. Most of what we're demonstrating here is how to make things work, rather than what will fail.
There was a problem hiding this comment.
FYI, this module currently runs in CI on Java 17
There was a problem hiding this comment.
And sorry, my point with this comment was that you may have a test that is misleadingly passing because of the --add-opens when it should not be.
I want to ensure that the conclusions reached in the article about Java version compatibility have been accurately tested.
If you think that CI is the wrong place to verify this, can you at least perform some manual testing with different JDK versions to verify what you are saying in the article is correct?
|
|
||
| @Before | ||
| public void before() { | ||
| environmentVariablesRule.set("system rules", "works"); |
There was a problem hiding this comment.
Out of curiosity, does this work equally well if the variable is set in the test itself, or does it have to be in a @Before?
There was a problem hiding this comment.
You can set an environment variable at any point. During before, System.getenv won't return the value you've just set, because env substitution isn't active. Within the test, the set method is "live"
| assertThat(System.getenv("testng")) | ||
| .isEqualTo("has environment variables"); |
There was a problem hiding this comment.
I would usually expect to see single element assertions on a single line, even when using AssertJ - but I guess this is a matter of style so if you feel strongly about it, feel free to leave it
d938e23 to
c152955
Compare
No description provided.