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

Locate actual wheels in WHEELS_DIR #2356

Merged
merged 1 commit into from Sep 20, 2023

Conversation

kalikiana
Copy link
Member

  • The wheels need to be stored in a way that they can be modified easily rather than pool which is volatile.
  • By default the pool is used as always.
  • When cloning the path is explicitly logged.

Fixes: https://progress.opensuse.org/issues/134390

@kalikiana kalikiana marked this pull request as ready for review August 30, 2023 07:55
@perlpunk
Copy link
Contributor

autotest.pm Outdated Show resolved Hide resolved
autotest.pm Outdated Show resolved Hide resolved
t/14-isotovideo.t Outdated Show resolved Hide resolved
@perlpunk
Copy link
Contributor

perlpunk commented Sep 1, 2023

https://github.com/os-autoinst/os-autoinst/actions/runs/6037760508/job/16382651406?pr=2356#step:3:894

Cannot read t/data/wheels_dir/writer/lib/Copy/Writer/Content.pm: No such file or directory at /usr/lib/perl5/site_perl/5.38.0/Devel/Cover/Report/Codecovbash.pm line 76.

I think t/data/wheels_dir needs to be added to this ignore:
https://github.com/os-autoinst/os-autoinst/blob/master/tools/invoke-tests#L47

Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

I think we need to test the case where the wheel would be located in the pool dir as well. As I wrote in my comment, this would fail with the PR in its current form.

@kalikiana kalikiana force-pushed the wheel_dir_20_logs branch 2 times, most recently from b8ebcc3 to 07d86f2 Compare September 6, 2023 08:41
t/14-isotovideo.t Outdated Show resolved Hide resolved
@kalikiana kalikiana force-pushed the wheel_dir_20_logs branch 3 times, most recently from 4193b23 to 7dc603e Compare September 7, 2023 08:45
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d8ad077) 95.07% compared to head (b3ebf1e) 95.08%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2356   +/-   ##
=======================================
  Coverage   95.07%   95.08%           
=======================================
  Files         155      155           
  Lines       15338    15367   +29     
=======================================
+ Hits        14582    14611   +29     
  Misses        756      756           
Files Changed Coverage Δ
OpenQA/Isotovideo/Runner.pm 100.00% <100.00%> (ø)
OpenQA/Isotovideo/Utils.pm 100.00% <100.00%> (ø)
autotest.pm 80.68% <100.00%> (+0.16%) ⬆️
t/14-isotovideo.t 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

autotest.pm Outdated Show resolved Hide resolved
@kalikiana
Copy link
Member Author

Ah! I forgot to add the docs update. Will push shortly.

doc/backend_vars.asciidoc Outdated Show resolved Hide resolved
doc/backend_vars.asciidoc Outdated Show resolved Hide resolved
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.

https://github.com/os-autoinst/os-autoinst/actions/runs/6015946938/job/16318885617#step:3:647 shows that previously the test took 62s. Now in https://github.com/os-autoinst/os-autoinst/actions/runs/6147257051/job/16678324796?pr=2356#step:3:647 75s. That is a runtime increase that I consider too big to be acceptable.

I created
https://progress.opensuse.org/issues/135521 for t/14-isotovideo.t running into timeouts already in local executions.

@kalikiana
Copy link
Member Author

https://github.com/os-autoinst/os-autoinst/actions/runs/6015946938/job/16318885617#step:3:647 shows that previously the test took 62s. Now in https://github.com/os-autoinst/os-autoinst/actions/runs/6147257051/job/16678324796?pr=2356#step:3:647 75s. That is a runtime increase that I consider too big to be acceptable.

I created https://progress.opensuse.org/issues/135521 for t/14-isotovideo.t running into timeouts already in local executions.

It's not much slower given for what it does, but fair enough. I checked again if we can avoid calling isotovideo here - and eventually realized that we can't test that libs from a wheel are loaded any other way.
So I'm going the route of splitting the test out into another file.

@perlpunk
Copy link
Contributor

I checked again if we can avoid calling isotovideo here - and eventually realized that we can't test that libs from a wheel are loaded any other way.

I'm wondering about that. Maybe with the right mocking it can be done?

@kalikiana
Copy link
Member Author

I checked again if we can avoid calling isotovideo here - and eventually realized that we can't test that libs from a wheel are loaded any other way.

I'm wondering about that. Maybe with the right mocking it can be done?

My concern was more to do with being able to reproduce the loading of the libraries. Good news is I was able to reproduce it via load_test_schedule. Bad news is it doesn't in fact add the inc path.

@perlpunk
Copy link
Contributor

perlpunk commented Sep 12, 2023

I played with it and got this working: perlpunk@cffabc6

  • I had to replace glob with File::Glob::bsd_glob and don't really know why. The normal glob had problems on the pool directory.
  • Instead of mocking checkout_git_repo_and_branch, which does the actual necessary push to @INC, I only mock clone_git
  • I created two different sets of wheels because once a certain Package::Name is loaded, perl won't load it again (unless you cheat and remove it from %INC). I decided to make it distinguishable with the added numeric suffix.

edit: and faster 7: [22:33:11] ./t/14-isotovideo.t ..................................... ok 58049 ms than before :)

t/14-isotovideo.t Outdated Show resolved Hide resolved
@mergify

This comment was marked as resolved.

- The wheels need to be stored in a way that they can be
modified easily rather than pool which is volatile.
- By default the pool is used as always.
- When cloning the path is explicitly logged.

Fixes: https://progress.opensuse.org/issues/134390
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.

7: [12:06:39] ./t/14-isotovideo.t ..................................... ok    59521 ms ( 0.00 usr  0.00 sys + 39.51 cusr  2.45 csys = 41.96 CPU)

@okurz okurz merged commit a91a4f7 into os-autoinst:master Sep 20, 2023
14 of 15 checks passed
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