Adapt PreferencesServiceTest#testLookupOrder() to UserScope #1087

[HeikoKlare]

Adapts the test case PreferencesServiceTest#testLookupOrder() to reflect the UserScope added to Equinox. Also improves the assertions in the test case.

Fixes #1087

[HeikoKlare]

@HannesWell This adapts to the UserScope added to Equinox in eclipse-equinox/equinox#446. From my understanding, simply adding the UserScope in the test case should be correct. Please let me know if anything else should be adapted.

[github-actions]

Test Results

   594 files  ±0     594 suites  ±0   40m 38s ⏱️ - 5m 27s
 3 865 tests ±0   3 843 ✅ +1   22 💤 ±0  0 ❌  - 1 
12 201 runs  ±0  12 040 ✅ +3  161 💤 ±0  0 ❌  - 3 

Results for commit 055326e. ± Comparison against base commit 142f58f.

[HannesWell]

Yes this looks good.
Thank you for the follow-up!

[HannesWell]

I'm not sure if we have already discused it, but what about the standard JUnit Assertions?

Suggested change

	assertThat(service.getDefaultLookupOrder(qualifier, key)).isNull();
	assertThat(service.getLookupOrder(qualifier, key)).containsExactly(defaultOrder);
	assertNull(service.getDefaultLookupOrder(qualifier, key));
	assertArrayEquals(defaultOrder, service.getLookupOrder(qualifier, key));

[HeikoKlare]

I have to admit that I really dislike assertEquals and assertArrayEquals because of their bad comprehensibility.
More objectively, I prefer assertThat over assertEquals/assertArrayEquals for several reasons:

• Easier to understand what is expected and what is actual. In particular because the order changed between JUnit 4 and JUnit 5, it became even harder to do and understand that correctly. I like the relieve myself as well as readers of the assertions from that burden.
• It is not restricted to comparing lists with lists and arrays with arrays but the type of data strucuture is basically irrelevant. In addition, it provides derivation such as ignoring the order. Using standard JUnit 5 assertions for the "basic" cases and assertThat for more complex cases will lead to an unnecessary and incomprehensible mixture (in my opinion).
• It provides better error messages, behaves better when one of the arrays is null (which you have to / should check separately with standard JUnit assertions, making the code more verbose) and allows you to chain assertions on a single object.

For that reasons I would like to migrate equals assertions to AssertJ (see also #177) for a JUnit 5 migration, as they have to be touched anyway (swapped actual/expected) and then benefit from everything mentioned above.

[HannesWell]

Sorry for the late reply.
I tend to agree that the assertj assertions are easier to comprehend, but on the other hand I prefer more standard-like solutions over 'custom' ones. And in this case I would consider the default Junit assertions more standard, with the usual advantages of more standard solutions.

But it would probably be good to discuss this in a wider audience and get general consensus to avoid edit-wars.

It is not restricted to comparing lists with lists and arrays with arrays but the type of data strucuture is basically irrelevant. In addition, it provides derivation such as ignoring the order. Using standard JUnit 5 assertions for the "basic" cases and assertThat for more complex cases will lead to an unnecessary and incomprehensible mixture (in my opinion).

I would not consider mixing so bad, it would also make it clear that it's not a simple assertion.

[HeikoKlare]

but on the other hand I prefer more standard-like solutions over 'custom' ones

I agree with you and in simple cases like assertNull(X), the standard assertion is probably better (or at least equally good) than assertThat(X).isNull(). In particular, in these cases the error messages are comparably expressive. Even though I would say that it does not really matter which of the two you use if you use AssertJ (or some other 3rd party library) in that project/file anyway.

I would not consider mixing so bad, it would also make it clear that it's not a simple assertion.

Also agreed. I withdraw my comment regard mixing "standard" and "custom" assertions.

But it would probably be good to discuss this in a wider audience and get general consensus to avoid edit-wars.

I already started a discussion on which custom assertion library we should use, to ensure that we do not start to mix them up: eclipse-platform/.github#177
Probably you are right that we should also have a quick agreement on when to use custom assertions at all. I am actually not concerned of edit wars, because as long as an assertion change improves something (like comphrensibility or error output), I consider it valuable. With respect to my change this discussion relates to I would say that it was 50% valuable (assertThat instead of assertArrayEquals) and 50% not (assertThat instead of assertNull).

A final point I want to make again is that I consider all the standard equality assertions really bad for two reasons:

1. They have pretty bad method signatures (taking two arguments of the same type), in which you easily mix up expected and actual values. Unfortunately, Java lacks a concept of extension methods like C# or Xtend, in which you can define something like isEqualTo(actual, expected) and use is in a fluent way as actual.IsEqualTo(expected).
2. The error messages of standard assertions are not very helpful. Consider this example (assertions to be used as alternatives):

int [] actual = new int[] { 1, 2, 3};
int [] expected = new int[] { 1, 2, 3, 4};
assertArrayEquals(actual, expected);
assertThat(actual).containsExactly(expected);

Standard assertions give you:

array lengths differ, expected: <3> but was: <4>

AssertJ gives you:

Expecting actual:
  [1, 2, 3]
to contain exactly (and in same order):
  [1, 2, 3, 4]
but could not find the following elements:
  [4]

[HeikoKlare]

I'll merge this one now to have the failing test fixed. But feel free to still comment on the usage of assertEquals vs, assertThat, @HannesWell.