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

Make fb BOM test lest flaky through parameterization #8085

Merged
merged 1 commit into from Aug 27, 2018

Conversation

Projects
None yet
3 participants
@andrewvc
Contributor

andrewvc commented Aug 24, 2018

The unicode BOM tests in test_harvester.py have been flaky (see #8032). It appears that the tests are potentially publishing the same event twice. The theory behind this patch is that the cause is the test for one BOM reading another BOM's log file.

This PR does two things to fix this:

  1. Parameterizes the tests so that each runs in its own directory
  2. Uses a more restrictive pattern for choosing the logfile input

The second fix really shouldn't do anything due to 1, but it is always better to have more specific and defensive code regardless.

Fixes #8032

Make fb BOM test lest flaky through parameterization
The unicode BOM tests in test_harvester.py have been flaky. It appears that the tests are potentially publishing the same event twice. The theory behind this patch is that the cause is the test for one BOM reading another BOM's log file.

This PR does two things to fix this:

1. Parameterizes the tests so that each runs in its own directory
2. Uses a more restrictive pattern for choosing the logfile input

The second fix really shouldn't do anything due to 1, but it is always better to have more specific and defensive code regardless.

Fixes #8032
@urso

This comment has been minimized.

Show comment
Hide comment
@urso

urso Aug 24, 2018

Collaborator

LGTM.

As the python tests are very stateful, we should not do table-driven tests as it has been done before this change.

The tests create a directory with inputs, outputs, configs, logs for debugging and inspection. Does the 'parameterized' decorate affect these directory names? Each test must have it's own unique directory.

Collaborator

urso commented Aug 24, 2018

LGTM.

As the python tests are very stateful, we should not do table-driven tests as it has been done before this change.

The tests create a directory with inputs, outputs, configs, logs for debugging and inspection. Does the 'parameterized' decorate affect these directory names? Each test must have it's own unique directory.

@andrewvc

This comment has been minimized.

Show comment
Hide comment
@andrewvc

andrewvc Aug 24, 2018

Contributor

@urso yes, each parameterized test uses its own dir. So, as I said, the two changes I made here are a bit belt + suspenders, but I think it's still nice.

Quoted below is the file layout from the BOM tests on this branch.

build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/coverage.cov
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/output
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/output/utf-16le-bom
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/registry
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/log
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/log/utf-16le-bomtest.log
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/utf-16le-bom.log
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/filebeat.yml
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/data
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/data/meta.json
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/coverage.cov
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/output
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/output/utf-16be-bom
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/utf-16be-bom.log
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/registry
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/log
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/log/utf-16be-bomtest.log
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/filebeat.yml
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/data
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/data/meta.json
build//system-tests/run/test_harvester.Test.test_bom_utf8
build//system-tests/run/test_harvester.Test.test_bom_utf8/coverage.cov
build//system-tests/run/test_harvester.Test.test_bom_utf8/output
build//system-tests/run/test_harvester.Test.test_bom_utf8/output/filebeat
build//system-tests/run/test_harvester.Test.test_bom_utf8/registry
build//system-tests/run/test_harvester.Test.test_bom_utf8/log
build//system-tests/run/test_harvester.Test.test_bom_utf8/log/bom8.log
build//system-tests/run/test_harvester.Test.test_bom_utf8/filebeat.log
build//system-tests/run/test_harvester.Test.test_bom_utf8/filebeat.yml
build//system-tests/run/test_harvester.Test.test_bom_utf8/data
build//system-tests/run/test_harvester.Test.test_bom_utf8/data/meta.json
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/coverage.cov
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/output
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/output/utf-8
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/utf-8.log
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/registry
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/log
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/log/utf-8test.log
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/filebeat.yml
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/data
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/data/meta.json
Contributor

andrewvc commented Aug 24, 2018

@urso yes, each parameterized test uses its own dir. So, as I said, the two changes I made here are a bit belt + suspenders, but I think it's still nice.

Quoted below is the file layout from the BOM tests on this branch.

build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/coverage.cov
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/output
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/output/utf-16le-bom
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/registry
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/log
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/log/utf-16le-bomtest.log
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/utf-16le-bom.log
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/filebeat.yml
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/data
build//system-tests/run/test_harvester.Test.test_boms_2_utf_16le_bom/data/meta.json
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/coverage.cov
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/output
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/output/utf-16be-bom
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/utf-16be-bom.log
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/registry
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/log
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/log/utf-16be-bomtest.log
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/filebeat.yml
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/data
build//system-tests/run/test_harvester.Test.test_boms_1_utf_16be_bom/data/meta.json
build//system-tests/run/test_harvester.Test.test_bom_utf8
build//system-tests/run/test_harvester.Test.test_bom_utf8/coverage.cov
build//system-tests/run/test_harvester.Test.test_bom_utf8/output
build//system-tests/run/test_harvester.Test.test_bom_utf8/output/filebeat
build//system-tests/run/test_harvester.Test.test_bom_utf8/registry
build//system-tests/run/test_harvester.Test.test_bom_utf8/log
build//system-tests/run/test_harvester.Test.test_bom_utf8/log/bom8.log
build//system-tests/run/test_harvester.Test.test_bom_utf8/filebeat.log
build//system-tests/run/test_harvester.Test.test_bom_utf8/filebeat.yml
build//system-tests/run/test_harvester.Test.test_bom_utf8/data
build//system-tests/run/test_harvester.Test.test_bom_utf8/data/meta.json
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/coverage.cov
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/output
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/output/utf-8
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/utf-8.log
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/registry
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/log
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/log/utf-8test.log
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/filebeat.yml
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/data
build//system-tests/run/test_harvester.Test.test_boms_0_utf_8/data/meta.json
@ruflin

ruflin approved these changes Aug 27, 2018

The reason I introduced these parameterized tests is exactly for having separate directories as otherwise files overwrite each other.

I like that these changes also make the tests more readable.

@ruflin ruflin merged commit 3273894 into elastic:master Aug 27, 2018

6 checks passed

CLA Commit author has signed the CLA
Details
Hound No violations found. Woof!
beats-ci Build finished.
Details
codecov/patch Coverage not affected when comparing 0b32c2d...e705586
Details
codecov/project 64.47% (+0.01%) compared to 0b32c2d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment