Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt PreferencesServiceTest#testLookupOrder() to UserScope #1087 #1088

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Adapt PreferencesServiceTest#testLookupOrder() to UserScope #1087 #1088

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 21 additions & 49 deletions ...tests.runtime/src/org/eclipse/core/tests/internal/preferences/PreferencesServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*******************************************************************************/
package org.eclipse.core.tests.internal.preferences;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
Expand Down Expand Up @@ -54,6 +55,7 @@
import org.eclipse.core.runtime.preferences.IScopeContext;
import org.eclipse.core.runtime.preferences.InstanceScope;
import org.eclipse.core.runtime.preferences.PreferenceFilterEntry;
import org.eclipse.core.runtime.preferences.UserScope;
import org.eclipse.core.tests.harness.FileSystemHelper;
import org.eclipse.core.tests.runtime.RuntimeTestsPlugin;
import org.junit.After;
Expand Down Expand Up @@ -218,6 +220,7 @@ public void testLookupOrder() {
String[] defaultOrder = new String[] {"project", //$NON-NLS-1$
InstanceScope.SCOPE, //
ConfigurationScope.SCOPE, //
UserScope.SCOPE, //
DefaultScope.SCOPE};
String[] fullOrder = new String[] {"a", "b", "c"};
String[] nullKeyOrder = new String[] {"e", "f", "g"};
Expand All @@ -230,70 +233,39 @@ public void testLookupOrder() {
() -> service.setDefaultLookupOrder(qualifier, key, new String[] { "a", null, "b" }));

// nothing set
String[] order = service.getDefaultLookupOrder(qualifier, key);
assertNull("1.0", order);
order = service.getLookupOrder(qualifier, key);
assertNotNull("1.1", order);
assertArrayEquals("1.2", defaultOrder, order);
assertThat(service.getDefaultLookupOrder(qualifier, key)).isNull();
assertThat(service.getLookupOrder(qualifier, key)).containsExactly(defaultOrder);
Comment on lines +236 to +237
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]


order = service.getDefaultLookupOrder(qualifier, null);
assertNull("1.3", order);
order = service.getLookupOrder(qualifier, null);
assertNotNull("1.4", order);
assertArrayEquals("1.5", defaultOrder, order);
assertThat(service.getDefaultLookupOrder(qualifier, null)).isNull();
assertThat(service.getLookupOrder(qualifier, null)).containsExactly(defaultOrder);

// set for qualifier/key
service.setDefaultLookupOrder(qualifier, key, fullOrder);
order = service.getDefaultLookupOrder(qualifier, key);
assertNotNull("2.2", order);
assertArrayEquals("2.3", fullOrder, order);
order = service.getLookupOrder(qualifier, key);
assertNotNull("2.4", order);
assertArrayEquals("2.5", fullOrder, order);
assertThat(service.getDefaultLookupOrder(qualifier, key)).containsExactly(fullOrder);
assertThat(service.getLookupOrder(qualifier, key)).containsExactly(fullOrder);

// nothing set for qualifier/null
order = service.getDefaultLookupOrder(qualifier, null);
assertNull("3.0", order);
order = service.getLookupOrder(qualifier, null);
assertNotNull("3.1", order);
assertArrayEquals("3.2", defaultOrder, order);
assertThat(service.getDefaultLookupOrder(qualifier, null)).isNull();
assertThat(service.getLookupOrder(qualifier, null)).containsExactly(defaultOrder);

// set for qualifier/null
service.setDefaultLookupOrder(qualifier, null, nullKeyOrder);
order = service.getDefaultLookupOrder(qualifier, null);
assertNotNull("4.0", order);
assertArrayEquals("4.1", nullKeyOrder, order);
order = service.getLookupOrder(qualifier, null);
assertNotNull("4.2", order);
assertArrayEquals("4.3", nullKeyOrder, order);
order = service.getDefaultLookupOrder(qualifier, key);
assertNotNull("4.4", order);
assertArrayEquals("4.5", fullOrder, order);
order = service.getLookupOrder(qualifier, key);
assertNotNull("4.6", order);
assertArrayEquals("4.7", fullOrder, order);
assertThat(service.getDefaultLookupOrder(qualifier, null)).containsExactly(nullKeyOrder);
assertThat(service.getLookupOrder(qualifier, null)).containsExactly(nullKeyOrder);
assertThat(service.getDefaultLookupOrder(qualifier, key)).containsExactly(fullOrder);
assertThat(service.getLookupOrder(qualifier, key)).containsExactly(fullOrder);

// clear qualifier/key (find qualifier/null)
service.setDefaultLookupOrder(qualifier, key, null);
order = service.getDefaultLookupOrder(qualifier, key);
assertNull("5.0", order);
order = service.getLookupOrder(qualifier, key);
assertNotNull("5.1", order);
assertArrayEquals("5.2", nullKeyOrder, order);
assertThat(service.getDefaultLookupOrder(qualifier, key)).isNull();
assertThat(service.getLookupOrder(qualifier, key)).containsExactly(nullKeyOrder);

// clear qualifier/null (find returns default-default)
service.setDefaultLookupOrder(qualifier, null, null);
order = service.getDefaultLookupOrder(qualifier, key);
assertNull("6.0", order);
order = service.getLookupOrder(qualifier, key);
assertNotNull("6.1", order);
assertArrayEquals("6.2", defaultOrder, order);

order = service.getDefaultLookupOrder(qualifier, null);
assertNull("6.3", order);
order = service.getLookupOrder(qualifier, null);
assertNotNull("6.4", order);
assertArrayEquals("6.5", defaultOrder, order);
assertThat(service.getDefaultLookupOrder(qualifier, key)).isNull();
assertThat(service.getLookupOrder(qualifier, key)).containsExactly(defaultOrder);
assertThat(service.getDefaultLookupOrder(qualifier, null)).isNull();
assertThat(service.getLookupOrder(qualifier, null)).containsExactly(defaultOrder);
}

@Test
Expand Down
Loading