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

tests: Use separate databases for unstable tests #4295

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

kalikiana
Copy link
Member

@kalikiana kalikiana commented Oct 12, 2021

This prevents failing and passing re-runs from being merged and allows individual databases to be inspected or dropped.

See: https://progress.opensuse.org/issues/99594

@perlpunk
Copy link
Contributor

perlpunk commented Oct 12, 2021

Hm, this might delete coverage data from previous tests, e.g. in unstable, like I mentioned in progress.
COVERDB_SUFFIX="_${CIRCLE_JOB}", and CIRCLE_JOB is just the job name, right? So it would be unstable.

We would have to add the test file name to the suffix

@kalikiana
Copy link
Member Author

kalikiana commented Oct 12, 2021

Hm, this might delete coverage data from previous tests, e.g. in unstable, like I mentioned in progress. COVERDB_SUFFIX="_${CIRCLE_JOB}", and CIRCLE_JOB is just the job name, right? So it would be unstable.

We would have to add the test file name to the suffix

We only have one db per job. export COVERDB_SUFFIX="_${CIRCLE_JOB}" is how it's defined. I would assume if fullstack fails once, cover_db_fullstack gets deleted and coverage of the next run will be "clean" in the context of the job.
For reference individual makefile targets look like test-with-database TIMEOUT_M=123 PROVE_ARGS="$$HARNESS t/api/*.t". We don't run test individually.

@perlpunk
Copy link
Contributor

What I mean is not fullstack, but unstable.
https://app.circleci.com/pipelines/github/os-autoinst/openQA/8037/workflows/c28ac1ca-ff44-460b-9060-2c08b507caec/jobs/75957

[...] -MDevel::Cover=-...,-db,cover_db_unstable [...]
   tools/retry prove -l [...] t/25-cache-service.t
[...] -MDevel::Cover=...,-db,cover_db_unstable [...]
   tools/retry prove -l [...] t/43-scheduling-and-worker-scalability.t
[...] -MDevel::Cover=...,-db,cover_db_unstable [...]
   tools/retry prove -l [...] t/ui/01-list.t

Here the coverdb directory is cover_db_unstable, and if t/ui/01-list.t fails, it will delete coverage of the previously ran t/25-cache-service.t and t/43-scheduling-and-worker-scalability.t

tools/retry Outdated Show resolved Hide resolved
@kalikiana kalikiana changed the title retry: Delete cover_db if defined tests: Use separate databases for unstable tests Oct 13, 2021
@kalikiana
Copy link
Member Author

For the record:

build-docs / tools/build-docs-ci

Checking for file conflicts: [-]Checking for file conflicts: [done]
(1/2) Installing: libpython2_7-1_0-2.7.18-7.55.1.x86_64 [/](1/2) Installing: libpython2_7-1_0-2.7.18-7.55.1.x86_64 [done]
(2/2) Installing: python-base-2.7.18-7.55.1.x86_64 [|](2/2) Installing: python-base-2.7.18-7.55.1.x86_64 [done]
Retry 3 of 3 …
Retrieving repository 'Update repository with updates from SUSE Linux Enterprise 15' metadata [\]error]
Repository 'Update repository with updates from SUSE Linux Enterprise 15' is invalid.
[repo-sle-update|http://download.opensuse.org/update/leap/15.3/sle/] Valid metadata not found at specified URL
History:
 - [|] Error trying to read from 'http://download.opensuse.org/update/leap/15.3/sle/'
 - Timeout exceeded when accessing 'http://download.opensuse.org/update/leap/15.3/sle/repodata/repomd.xml'.

Please check if the URIs defined for this repository are pointing to a valid repository.
Warning: Skipping repository 'Update repository with updates from SUSE Linux Enterprise 15' because of the above error.
Some of the repositories have not been refreshed because of an error.

unstable / Persisting to workspace

The specified paths did not match any files in /home/squamata/project

@Martchus
Copy link
Contributor

So the test coverage from unstable tests will no longer show up in the normal coverage report? And don't we need the same for test-fullstack and test-fullstack-unstable which both are also running with a RETRY parameter greater than 1?

@perlpunk
Copy link
Contributor

So the test coverage from unstable tests will no longer show up in the normal coverage report?

it will. All directories called _cover_db_* will be read from the final report.

And don't we need the same for test-fullstack and test-fullstack-unstable which both are also running with a RETRY parameter greater than 1?

in those we only execute one test, in unstable currently three.
yes, as soon as we add a for-loop in other jobs, we need to do the same there.

@perlpunk
Copy link
Contributor

This prevents failing and passing re-runs from being merged and allows individual databases to be inspected or dropped.

the current PR description is a bit confusing. the directory just changes to a directory per test file name, which will allow individual deletion without deleting the data of the other tests. that will be done in the followup #4299

@Martchus
Copy link
Contributor

it will.

Good.

yes, as soon as we add a for-loop in other jobs, we need to do the same there.

Not sure what you mean by for-loop. We set the RETRY parameter when running those tests. I though this was the problem.

@perlpunk
Copy link
Contributor

perlpunk commented Oct 13, 2021

The problem is that we don't delete coverage data for failing tests, when we retry.

So, we have to delete coverage if we retry.
So far so good. We can do that in the retry script - simply delete the cover_db_circleci_jobname folder.
That I will do in my followup.

The problem with unstable is that we run tools/retry in a for loop for 3 tests.

All those three tests write their coverage in cover_db_circleci_unstable.

test1.t suceeds
test2.t fails, then thanks to retry suceeds.
test3.t suceeds

Now, if test2.t would delete cover_db_circleci_unstable after the fail, then the coverage data for test1.t would be deleted too. We don't want that.

This PR makes sure that in the case we run tools/retry in such a for loop every test file gets their own directory, e.g. cover_db_jobname_test1.t . So if that has to be deleted, the data from the other tests are not deleted.

@kalikiana
Copy link
Member Author

Not sure what you mean by for-loop. We set the RETRY parameter when running those tests. I though this was the problem.

To clarify, test-unstable contains a for loop which calls test-with-database with different .pl files. Other targets call exactly one list of tests.

Makefile Outdated Show resolved Hide resolved
@Martchus
Copy link
Contributor

Now I understand it but the change suggested by @perlpunk is still required.

This prevents failing and passing re-runs from being merged and allows
individual databases to be inspected or dropped.

See: https://progress.opensuse.org/issues/99594
@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #4295 (648acf8) into master (bfb676e) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4295      +/-   ##
==========================================
- Coverage   97.95%   97.94%   -0.01%     
==========================================
  Files         370      370              
  Lines       33488    33488              
==========================================
- Hits        32803    32801       -2     
- Misses        685      687       +2     
Impacted Files Coverage Δ
lib/OpenQA/Worker/WebUIConnection.pm 93.13% <0.00%> (-0.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfb676e...648acf8. Read the comment docs.

@mergify mergify bot merged commit 3999271 into os-autoinst:master Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants