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

Reduce use of project dir to be more flexible regarding custom test runs #2410

Merged
merged 4 commits into from
Oct 23, 2019

Conversation

Martchus
Copy link
Contributor


The os-autoinst part is still missing.

@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #2410 into master will increase coverage by 0.08%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2410      +/-   ##
==========================================
+ Coverage   87.13%   87.22%   +0.08%     
==========================================
  Files         169      169              
  Lines       11086    11075      -11     
==========================================
  Hits         9660     9660              
+ Misses       1426     1415      -11
Impacted Files Coverage Δ
lib/OpenQA/Worker/Engines/isotovideo.pm 93.8% <ø> (-0.03%) ⬇️
lib/OpenQA/Schema/Result/JobModules.pm 85.47% <0%> (+2.68%) ⬆️
lib/OpenQA/Schema/Result/Needles.pm 71.08% <100%> (+4.41%) ⬆️
lib/OpenQA/Utils.pm 92.94% <100%> (+0.48%) ⬆️
lib/OpenQA/WebAPI/Controller/File.pm 91.39% <100%> (-0.27%) ⬇️
lib/OpenQA/WebAPI/Controller/Step.pm 64.94% <90.9%> (+2.21%) ⬆️

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 a3da178...8a682cc. Read the comment docs.

@Martchus Martchus marked this pull request as ready for review October 21, 2019 16:09
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

your tests have covered some diff, would you like to get to a 100% diff coverage?

lib/OpenQA/Schema/Result/Needles.pm Outdated Show resolved Hide resolved
@Martchus
Copy link
Contributor Author

Seems like OpenQA::Utils::needle_info also relies on the needle paths being relative to the share dir so this is not working.

* Locate needles relative to the job's needles directory (keeping legacy
  way to locate needles as a fallback for compatibility)
* Don't try to update needles which can not be located
* Don't use OpenQA::Utils::needle_info() to locate needles in file
  API
    * It is not required to parse the needle JSON here
    * The comment stating the needle path comes from the database
      was wrong. It was only deduced via needledir() which is now
      used directly.
* No longer pass the project dir to isotovideo
* See https://progress.opensuse.org/issues/56789
  and https://progress.opensuse.org/issues/58184
@Martchus
Copy link
Contributor Author

Martchus commented Oct 23, 2019

I changed the needle lookup in OpenQA::Utils::needle_info to be consistent with the lookup when updating needles (removing the redundancy). That works now (did some manual tests and ran some relevant unit tests). I'll check the test coverage and try to get 100 % diff coverage.

By the way, the local my %needles; variable in store_needle_infos is populated but never used so I removed it (just in case one is wondering about the diff).

The function is only used within that controller anymore.
This seems to be required at least for the test fixture needles
used within our test suite. The previous needle_info() function
had the same fallback.
The _basic_needle_info function already produces a log message
in the different error cases.
@AdamWill
Copy link
Contributor

AdamWill commented Nov 1, 2019

This breaks needlediff in the web UI for needles in subdirectories:

https://progress.opensuse.org/issues/58959

AdamWill added a commit to AdamWill/openQA that referenced this pull request Nov 1, 2019
As discussed in the POO, this was broken by PR os-autoinst#2410 commit
36aa974 - it assumes you can always find a needle simply by
sticking the needle filename on the end of a `needledir` call,
but you can't, needles are allowed to be in subdirectories of
needledir. This should hopefully fix that without breaking
the custom run case by using the *whole* of the JSON file path -
we just figure out the subdirectory component from it. This works
for me in the 'needle is in a subdirectory of the normal needle
dir' case, but I didn't test it in the custom run case.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/openQA that referenced this pull request Nov 4, 2019
As discussed in the POO, this was broken by PR os-autoinst#2410 commit
36aa974 - it assumes you can always find a needle simply by
sticking the needle filename on the end of a `needledir` call,
but you can't, needles are allowed to be in subdirectories of
needledir. This should hopefully fix that without breaking
the custom run case by using the *whole* of the JSON file path -
we just figure out the subdirectory component from it. This works
for me in the 'needle is in a subdirectory of the normal needle
dir' case, but I didn't test it in the custom run case.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/openQA that referenced this pull request Nov 5, 2019
As discussed in the POO, this was broken by PR os-autoinst#2410 commit
36aa974 - it assumes you can always find a needle simply by
sticking the needle filename on the end of a `needledir` call,
but you can't, needles are allowed to be in subdirectories of
needledir. This should hopefully fix that without breaking
the custom run case by using the *whole* of the JSON file path -
we just figure out the subdirectory component from it. This works
for me in the 'needle is in a subdirectory of the normal needle
dir' case, but I didn't test it in the custom run case.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/openQA that referenced this pull request Nov 5, 2019
As discussed in the POO, this was broken by PR os-autoinst#2410 commit
36aa974 - it assumes you can always find a needle simply by
sticking the needle filename on the end of a `needledir` call,
but you can't, needles are allowed to be in subdirectories of
needledir. This should hopefully fix that without breaking
the custom run case by using the *whole* of the JSON file path -
we just figure out the subdirectory component from it. This works
for me in the 'needle is in a subdirectory of the normal needle
dir' case, but I didn't test it in the custom run case.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/openQA that referenced this pull request Nov 5, 2019
As discussed in the POO, this was broken by PR os-autoinst#2410 commit
36aa974 - it assumes you can always find a needle simply by
sticking the needle filename on the end of a `needledir` call,
but you can't, needles are allowed to be in subdirectories of
needledir. This should hopefully fix that without breaking
the custom run case by using the *whole* of the JSON file path -
we just figure out the subdirectory component from it. This works
for me in the 'needle is in a subdirectory of the normal needle
dir' case, but I didn't test it in the custom run case.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
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

3 participants