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

Better failure message for characterization test #954

Merged
merged 12 commits into from
Oct 18, 2019
+20 −8
Merged

Better failure message for characterization test #954

Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2e7f09a
Use Truth for better failure message
elharo Oct 8, 2019
521c888
Use Truth for better failure message
elharo Oct 8, 2019
497b833
Merge branch 'master' into repotest
elharo Oct 8, 2019
acc8ed4
remove guard
elharo Oct 9, 2019
052d65e
manually compute list differences
elharo Oct 9, 2019
e194b02
rename
elharo Oct 9, 2019
9f7a78b
Merge branch 'master' into repotest
elharo Oct 14, 2019
00ab4e9
use guarded containsExactly
Oct 15, 2019
1a88000
merge master
Oct 18, 2019
f729be7
use containsExactlyElementsIn
Oct 18, 2019
9ddecf3
add extra versions
Oct 18, 2019
c6965c1
Merge branch 'master' into repotest
elharo Oct 18, 2019
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
24 changes: 17 additions & 7 deletions ...s/src/test/java/com/google/cloud/tools/opensource/dependencies/RepositoryUtilityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;

import org.eclipse.aether.RepositorySystem;
import org.eclipse.aether.RepositorySystemSession;
import org.eclipse.aether.artifact.Artifact;
Expand Down Expand Up @@ -57,16 +58,25 @@ public void testReadBom_coordinates() throws ArtifactDescriptorException {
}

@Test
public void testReadBom_path() throws MavenRepositoryException {
public void testReadBom_path() throws MavenRepositoryException, ArtifactDescriptorException {
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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

ImmutableList<Artifact> currentArtifacts = currentBom.getManagedDependencies();
ImmutableList<Artifact> oldArtifacts = oldBom.getManagedDependencies();

String coordinates = currentBom.getCoordinates();
Truth.assertThat(coordinates).startsWith("com.google.cloud:libraries-bom:");
Truth.assertThat(coordinates).endsWith("-SNAPSHOT");

ImmutableList<Artifact> artifacts = bom.getManagedDependencies();
Assert.assertEquals(219, artifacts.size());
String coordinates = bom.getCoordinates();
Assert.assertTrue(coordinates.startsWith("com.google.cloud:libraries-bom:"));
Assert.assertTrue(coordinates.endsWith("-SNAPSHOT"));
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (currentArtifacts.size() != 217) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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

@Test
Expand Down