Better failure message for characterization test #954

elharo · 2019-10-08T19:21:51Z

netdpb · 2019-10-08T20:03:30Z

...cies/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java

    Path pomFile = Paths.get("..", "boms", "cloud-oss-bom", "pom.xml");

-    Bom bom = RepositoryUtility.readBom(pomFile);
+    Bom currentBom = RepositoryUtility.readBom(pomFile);
+    Bom oldBom = RepositoryUtility.readBom("com.google.cloud:libraries-bom:2.5.0");


Is this something that you'll have to update manually for each release?

Whenever a new release changes the total number of artifacts pulled in. What this PR does is show us what's changed, instead of just giving a number.

Originally this test was to check that we could could correctly read the BOM from the file system at HEAD, which was surprisingly hard compared to reading it from the network. We don't really expect the number of jars pulled in to stay the same from release to release, and have had to change this value multiple times already. There might be a better way to do this than a unit test. Suggestions appreciated.

I think it's okay to have a regression test like this, even if it means changing it when it fails.

netdpb · 2019-10-08T20:04:53Z

...cies/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java

-    ImmutableList<Artifact> artifacts = bom.getManagedDependencies();
-    Assert.assertEquals(217, artifacts.size());
-    String coordinates = bom.getCoordinates();
+    String coordinates = currentBom.getCoordinates();
    Assert.assertTrue(coordinates.startsWith("com.google.cloud:libraries-bom:"));


What about assertThat(coordinates).startsWith(...) and .endsWith(...)?

endsWith changes every time we bump the version.

I just mean literally to replace this line's and the next line's assertTrue(coordinates.{starts,ends}With( with assertThat(coordinates).{starts,ends}With(. You'll get better error messages.

You could even just use assertThat(coordinates).matches("com\.google\.cloud:libraries-bom:.*-SNAPSHOT") for both.

netdpb · 2019-10-08T20:27:38Z

...cies/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java

+    // However sometimes this list does change. If so, we want to 
+    // output the specific difference so we can manually verify whether
+    // the changes make sense. When they do make sense, we update the test. 
+    if (currentArtifacts.size() != 217) {


Is 217 the current size? Why not just always assert? If they haven't changed the test will pass.

I'm still a bit confused. The purpose of this guard (!= 217) is to avoid asserting that currentArtifacts is the same as oldArtifacts unless we think the size has changed. But they could still differ even if the size hasn't changed.

What you have here could induce a false negative if the actual set of dependencies changes without the size changing. Why allow that?

netdpb · 2019-10-09T15:04:46Z

...cies/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java

+    // This is a characterization test to verify that the managed dependencies haven't changed.
+    // However sometimes this list does change. If so, we want to 
+    // output the specific difference so we can manually verify whether
+    // the changes make sense. When they do make sense, we update the test. 


How exactly will we update this test when we need to? We won't be able to assert against any released BOM version. We won't want to assert that the snapshot's set of managed dependencies equals the snapshot's set of managed dependencies.

And now You've reminded me that's why the guard was there. I'll put it back in.

Hmm. I don't understand how the guard helps.

With the guard:

If the dependencies haven't changed at all, the test will correctly pass.

If the set of dependencies has changed, but there are still 217, the test will pass. Is this the behavior you want?

If the set of dependencies has changed, and so has the size of the set, the test will fail.

Without the guard:

If the dependencies haven't changed at all, the test will correctly pass.

If the set of dependencies has changed, but there are still 217, the test will fail.

If the set of dependencies has changed, and so has the size of the set, the test will fail.

Totally orthogonal to the guard question: How do you expect to update this test when it fails?

"If the set of dependencies has changed, but there are still 217, the test will pass. Is this the behavior you want?"

Not in an ideal world, but it's the best we can do for now short of encoding the entire list of expected dependencies into the test somewhere.

Does Truth expose its list comparison logic in the public API? I don't need to assert the contents of the list as long as I can find out what's changed when they differ. Or maybe I should just convert to sets and do a difference.

it's the best we can do for now short of encoding the entire list of expected dependencies into the test somewhere.

Hmm. Now I'm not sure what this assertion is testing for. Are you worried that someone will inadvertently introduce an extra element into the BOM? If so, then I'm not sure that a unit test is the best way to achieve that. Someone would have to review the XML doc change, and that someone would have to review the test change as well. Is it just a change-detector test?

Or are you testing that RepositoryUtility.readBom() correctly reads a BOM file? If so, then maybe we really just need a test BOM that doesn't need to change with the real BOM.

Does Truth expose its list comparison logic in the public API? I don't need to assert the contents of the list as long as I can find out what's changed when they differ. Or maybe I should just convert to sets and do a difference.

containsExactlyElementsIn() should do a good job of reporting only missing and extra elements. It doesn't care about order, although it does care about multiplicity. If you wanted to care about order, you could do containsExactlyElementsIn(expected).inOrder().

The primary purpose for this test is to verify that we're reading a BOM from the local file system correctly. When we first wrote this test, that was surprisingly hard to do.

Yes, this is a change detector test, though that's not it's primary purpose. The reason for this PR is to make the actual changes more obvious so we can manually verify that they're expected changes in the BOM and not malfunctioning code.

I tried to use containsExactlyElementsIn but it proved complex and poorly documented, so I switched over to writing my own. Some things I needed to be public to use it outside of Truth.assertThat weren't public:

google/truth#634

What was the trouble with containsExactlyElementsIn?

I'm still having trouble understanding how a change-detector test helps if the changes it would detect have to be literal changes in a configuration file in the same PR. Maybe we could chat in person?

Without the change detector test, the changes can go in without anyone noticing. The change detector test alerts the dev and reviewer to the change so they can sanity check it.

Wouldn't the changes to the XML file be just as visible to the reviewer as changes to this test?

The reason for this PR is to make the actual changes more obvious so we can manually verify that they're expected changes in the BOM and not malfunctioning code.

If the only way the artifacts can change is by a change to the XML file itself, not to code, then how does this test prevent unexpected changes to the BOM from malfunctioning code?

Anyway, we can have this conversation separately from this PR. Not blocking.

The changes to the XML file would not be as visible to the reviewer or especially the developer as changes to this test. This shows the actual dependency changes. What the change to the XML file shows is that we went from google-cloud-bom:0.102.0 to 0.115.0, nothing more.

Currently this test shows that that change added a couple of dependencies:

RepositoryUtilityTest.testReadBom_path:66 expected:<217> but was:<219>

However, without this change we don't see which dependencies were added. That's what this PR adds.

netdpb · 2019-10-14T15:28:48Z

...cies/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java

+        Set<Artifact> current = Sets.newHashSet(currentArtifacts);
+        Set<Artifact> old = Sets.newHashSet(oldArtifacts);
+        SetView<Artifact> currentMinusOld = Sets.difference(current, old);
+        String added = Joiner.on(", ").join(currentMinusOld);
+        SetView<Artifact> oldMinusCurrent = Sets.difference(old, current);
+        String subtracted = Joiner.on(", ").join(oldMinusCurrent);
+        Assert.fail("Dependency tree changed.\n  Added " + added + "\n  Removed" + subtracted);


Regardless of the conversation above, I don't see how this is any better than what you had before, which was one line, and shows missing and unexpected elements.

assertThat(currentArtifacts).containsExactlyElementsIn(oldArtifacts);

As you pointed out, that wasn't guarded and couldn't be changed when we needed it to be.

netdpb · 2019-10-14T15:37:09Z

...cies/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java

+    // This is a characterization test to verify that the managed dependencies haven't changed.
+    // However sometimes this list does change. If so, we want to 
+    // output the specific difference so we can manually verify whether
+    // the changes make sense. When they do make sense, we update the test. 


Wouldn't the changes to the XML file be just as visible to the reviewer as changes to this test?

The reason for this PR is to make the actual changes more obvious so we can manually verify that they're expected changes in the BOM and not malfunctioning code.

If the only way the artifacts can change is by a change to the XML file itself, not to code, then how does this test prevent unexpected changes to the BOM from malfunctioning code?

Anyway, we can have this conversation separately from this PR. Not blocking.

netdpb · 2019-10-16T14:15:44Z

...cies/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java

+    // output the specific difference so we can manually verify whether
+    // the changes make sense. When they do make sense, we update the test. 
+    if (currentArtifacts.size() != 217) {
+        Truth.assertThat(currentArtifacts).containsExactly(oldArtifacts);


Suggested change

Truth.assertThat(currentArtifacts).containsExactly(oldArtifacts);

Truth.assertThat(currentArtifacts).containsExactlyElementsIn(oldArtifacts);

containsExactly(Object...) takes varargs of expected elements. You want containsExactlyElementsIn(Iterable<?>), which takes an iterable of expected elements.

I think Truth actually suggests that in its failure method.

And now you've reminded me why I prefer writing out the comparison instead of using yet another library. These method names give me no indication of when I should use containsExactly or containsExactlyElementsIn. Without looking at the docs I would expect them to do the same thing. I do happen to remember that order is not significant here, but again there's nothing in the method names to tell me that.

If I simply write the code using ordinary Java classes, it's very obvious what is being compared and how and what the conditions are. This change is shorter than the pure-Java equivalent, but it is more opaque. :-(

elharo added 2 commits October 8, 2019 15:19

Use Truth for better failure message

2e7f09a

Use Truth for better failure message

521c888

googlebot added the cla: yes label Oct 8, 2019

Merge branch 'master' into repotest

497b833

elharo requested a review from netdpb October 8, 2019 19:40

elharo added the Testing label Oct 8, 2019

netdpb reviewed Oct 8, 2019

View reviewed changes

remove guard

acc8ed4

netdpb reviewed Oct 9, 2019

View reviewed changes

elharo added 2 commits October 9, 2019 13:53

manually compute list differences

052d65e

rename

e194b02

elharo mentioned this pull request Oct 11, 2019

IterableSubject constructor documentation for check method google/truth#634

Open

netdpb reviewed Oct 14, 2019

View reviewed changes

elharo and others added 2 commits October 14, 2019 15:41

Merge branch 'master' into repotest

9f7a78b

use guarded containsExactly

00ab4e9

netdpb reviewed Oct 16, 2019

View reviewed changes

Elliotte Rusty Harold added 2 commits October 18, 2019 09:39

merge master

1a88000

use containsExactlyElementsIn

f729be7

netdpb approved these changes Oct 18, 2019

View reviewed changes

Elliotte Rusty Harold and others added 2 commits October 18, 2019 10:20

add extra versions

9ddecf3

Merge branch 'master' into repotest

c6965c1

elharo merged commit d955279 into master Oct 18, 2019

elharo deleted the repotest branch October 18, 2019 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better failure message for characterization test #954

Better failure message for characterization test #954

elharo commented Oct 8, 2019 •

edited

Loading

netdpb Oct 8, 2019

elharo Oct 8, 2019

netdpb Oct 9, 2019

netdpb Oct 8, 2019

elharo Oct 8, 2019

netdpb Oct 9, 2019

elharo Oct 9, 2019

netdpb Oct 8, 2019

elharo Oct 8, 2019

netdpb Oct 9, 2019

elharo Oct 9, 2019

netdpb Oct 9, 2019

elharo Oct 9, 2019

netdpb Oct 9, 2019

elharo Oct 9, 2019

netdpb Oct 9, 2019

elharo Oct 9, 2019

netdpb Oct 11, 2019

elharo Oct 14, 2019

netdpb Oct 14, 2019

elharo Oct 14, 2019

netdpb Oct 14, 2019

elharo Oct 14, 2019

elharo Oct 15, 2019

netdpb Oct 14, 2019

netdpb Oct 16, 2019

elharo Oct 18, 2019

	Truth.assertThat(currentArtifacts).containsExactly(oldArtifacts);
	Truth.assertThat(currentArtifacts).containsExactlyElementsIn(oldArtifacts);

Better failure message for characterization test #954

Better failure message for characterization test #954

Conversation

elharo commented Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

With the guard:

Without the guard:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo commented Oct 8, 2019 •

edited

Loading