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.

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

ZOOKEEPER-4232: InvalidSnapshotTest corrupts its own test data #1629

Conversation

ztzg
Copy link
Contributor

@ztzg ztzg commented Mar 9, 2021

InvalidSnapshotTest.testSnapshot starts an instance of
ZooKeeperServer on the version-controlled resources/data/invalidsnap
directory, which, as a side-effect, "fixes" the following
snapshot—which was broken on purpose (see ZOOKEEPER-367):

zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f

This status quo creates a number of problems:

  1. It makes the test ineffective after the first run;
  2. The file shows as modified in version control tools, which can be
    annoying;
  3. The "fixed" snapshot can end up being committed by mistake,
    invalidating the test.

(#3 is not theoretical; that "fixed" snapshot frequently shows up in
pull requests, and was recently merged into master.)

ztzg added 2 commits March 9, 2021 11:18
Without this, the ZooKeeperServer "fixes" the 'snapshot.83f' reference
file, which:

 1. Makes the test ineffective after the first run;
 2. Causes the file to show as modified in version control tools;
 3. Can end up committed by accident.

This patch creates a temporary copy of the data directory.
Copy link
Contributor

@arshadmohammad arshadmohammad left a comment

Choose a reason for hiding this comment

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

LGTM +1

asfgit pushed a commit that referenced this pull request Mar 9, 2021
`InvalidSnapshotTest.testSnapshot` starts an instance of
`ZooKeeperServer` on the version-controlled `resources/data/invalidsnap`
directory, which, as a side-effect, \"fixes\" the following
snapshot—which was broken on purpose (see ZOOKEEPER-367):

`zookeeper-server/src/test/resources/data/invalidsnap/version-2/snapshot.83f`

This status quo creates a number of problems:

1.  It makes the test ineffective after the first run;
2.  The file shows as modified in version control tools, which can be
    annoying;
3.  The \"fixed\" snapshot can end up being committed by mistake,
    invalidating the test.

(\#3 is not theoretical; that \"fixed\" snapshot frequently shows up in
pull requests, and was recently merged into `master`.)

Author: Damien Diederen <dd@crosstwine.com>

Reviewers: Mohammad Arshad <arshad@apache.org>

Closes #1629 from ztzg/ZOOKEEPER-4232-invalid-snapshot-is-invalid-x-3.5 and squashes the following commits:

5286405 [Damien Diederen] ZOOKEEPER-4232: Ensure that ZOOKEEPER-367 test data fails to parse
680c57a [Damien Diederen] ZOOKEEPER-4232: Run InvalidSnapshotTest on a copy of test data
@arshadmohammad
Copy link
Contributor

Merged

@ztzg
Copy link
Contributor Author

ztzg commented Mar 9, 2021

Thank you, @arshadmohammad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants