Tests that depend on specific error messages are brittle.

[garretwilson]

It's not a good idea to depend on specific error messages in an exception. For example in ExtendedYamlLintConfigTest:

        try {
            new YamlLintConfig("extends:\n  - foo");
            fail("Invalid config not identified");
        } catch (YamlLintConfigException e) {
            assertEquals("invalid config: java.util.ArrayList cannot be cast to java.lang.String", e.getMessage());
        }

You want to check that the correct exception will be thrown, yes. But error messages can easily change, and shouldn't be part of the semantics of the error. They are just useful for a user. But you shouldn't depend on them.

The above example is failing for me on Java 11, because Java 11 seems to have changed the error message:

ExtendedYamlLintConfigTest.testWrongExtend:40 expected:<invalid config: [java.util.ArrayList cannot be cast to java.lang.String]> but was:<invalid config: [class java.util.ArrayList cannot be cast to class java.lang.String (java.util.ArrayList and java.lang.String are in module java.base of loader 'bootstrap')]>

If you need to check for some specific error condition, you should add this condition as some property/attribute of the returned exception, or if it is a large category of things, make a new exception class or subclass. But checking for a specific error message makes the test brittle.

I'm going to have to do the changes for issue #20 "blind" to some extent (unless I want to switch to using a Java 8 JVM, which I shouldn't have to do, as Java 11 runs most Java 8 projects just fine).

[sbaudoin]

Hello,

Yes I agree with you, all the more so I used to explain such things to junior developers a long time ago, but... So 3 options for me here:

• Stick with Java 8: this is the version used in the build script
• Disable the tests for the complete build and Travis will deal with them to build the JAR
• Change the test to something more generic. I'm not willing to flavor the exception depending on the error (it's not the purpose); adding a property would not change a lot of thing: as the purpose of the test is to cover the lines 176...183 of YamlLingConfig, which are for the general case, I'd change the test to the generic purpose:

  assertTrue(e.getMessage().startsWith("invalid config: "));

I'd go for option 3 and slightly change the exception message as follows (line 181 of YamlLintConfig):

                throw new YamlLintConfigException("invalid config (unexpected error): " + e.getMessage(), e);

and have the following test:

assertTrue(e.getMessage().startsWith("invalid config (unexpected error): "));

[garretwilson]

Stick with Java 8: this is the version used in the build script

Ah, but there's a catch here. The build just specifies 1) that the source code is in Java 8, and 2) that the build will produce class files using Java 8 format. But what JVM is used to run the code is a different matter altogether! Here I think you're getting lucky and the GitHub tests (I assume you have that configured, but I haven't personally used it) uses the Java 8 runtime for running the tests. If GitHub (or another developer like me) decided to use Java 11 to run the tests (which we should be able to do!), it would break (as it did for me).

I'd go for option 3 and slightly change the exception message …

You're saying you would change the test to check only the part of the string you have complete control over. OK, that's better than nothing. I personally still think it's brittle and it's not what I would do, but the important thing is that you have taken the JVM out of the equation and stopped it from breaking on different systems. 👍

(I personally think that if the "flavor" is so important that it needs to be checked, it needs to be moved to something semantic like an exception flavor or a field, e.g. getErrorCode(), maybe with a enum. What happens when you translate these error methods to Mandarin or something? But anyway you already got the idea. Cheers!)

[sbaudoin]

I launch Maven from IntelliJ that runs with its custom JDK 11, so that's odd. Travis uses the openjdk8 for the build, so no issue with it.

Anyway, fixed but still in a brittle way...

[garretwilson]

I launch Maven from IntelliJ that runs with its custom JDK 11, so that's odd. Travis uses the openjdk8 for the build, so no issue with it.

The test ran fine for me in Eclipse as well; when Eclipse imported the Maven project it automatically opted for the Java 8 I had configured as one of my Eclipse JREs. I installed it a long time ago; I guess it is the Oracle version:

java version "1.8.0_162"
Java(TM) SE Runtime Environment (build 1.8.0_162-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.162-b12, mixed mode)

But it broke when I ran the tests from Maven on the command line, because currently my default system Java is:

openjdk 11.0.5 2019-10-15
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.5+10)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.5+10, mixed mode)