fix: Test failure in TestModule.testModuleNames() (#4285)

[xzel23]

This fixes #4285. The test relied on the order of entries returned by File#listFiles(), which is unspecified (thank you @SirYwell ).

[slarse]

Hi @xzel23, thanks for fixing the flaky test! I propose we go one step further to improve it while we're at it, see comment :)

[slarse]

While you're poking about here anyway, how about improving it just a little bit more? This test is a bit weak (because it leaves the possibility of there being other module names in the list), and it also isn't entirely clear about its purpose.

I propose we assert on the entire list. And instead of a list use a set, because we care only about distinct module names.

Suggested change

	List<String> list = Arrays.stream(cu).map(CompilationUnit::getModuleName).map(String::copyValueOf).collect(Collectors.toList());
	assertTrue(list.contains("foo"));
	assertTrue(list.contains("bar"));
	Set<String> moduleNames = Arrays.stream(cu)
	.map(CompilationUnit::getModuleName)
	.map(String::valueOf)
	.collect(Collectors.toSet());
	assertThat(moduleNames, equalTo(Set.of("bar", "foo")));

[xzel23]

done. I replaced the suggested deprecated assertThat with a non-deprecated one.

[slarse]

Hmm, I didn't suggest the deprecated assertThat, I just didn't specify which one to use (in fact I didn't even remember that JUnit 4 had a built-in assertThat). You've imported both, so I suspect your IDE auto-imported a bunch of stuff. See review comments.

[slarse]

Not needed

Suggested change

	import org.hamcrest.CoreMatchers;
	import org.hamcrest.MatcherAssert;

[slarse]

I don't think ArrayList is needed anymore.

Suggested change

import java.util.ArrayList;

[slarse]

Suggested change

import static org.junit.Assert.assertThat;

[slarse]

Suggested change

	MatcherAssert.assertThat(list, is(Set.of("foo", "bar")));
	assertThat(list, is(Set.of("foo", "bar")));

[xzel23]

My IDE had somehow lost track of the imports and marked everything as missing, so I didn't see the unused exports. Fixed after restart. I have cleaned it up and added a static import for the hamcrest assertThat as suggested by the junit deprecationcomment. Hope it's alright now.

[slarse]

Yeah, I feel you. I find that most IDEs with default settings are over-eager when it comes to auto-importing stuff, it gets rather confusing rather quickly.

[slarse]

Thanks @xzel23, this is a much more robust (and non-flaky) test!