-
Notifications
You must be signed in to change notification settings - Fork 217
Fully Cover OpenQA::WebAPI::Plugin::MemoryLimit #6693
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
base: master
Are you sure you want to change the base?
Conversation
01ee10f
to
a323eda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid the use of "assignation" in this context as "assignment" is much more conventional here. The commit message should probably just be "Use signatures in memory limit plugin" anyway, though.
t/api/14-plugin_memory_limit.t
Outdated
use_ok('OpenQA::WebAPI::Plugin::MemoryLimit'); | ||
|
||
my $app = Mojolicious->new; | ||
OpenQA::WebAPI::Plugin::MemoryLimit->register($app); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is still not setting max_rss_limit
to something > 0 and even if it would the relevant code wouldn't be executed as the test needed to run the event loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh you are right, but I was sure to have it at one point 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your new version still doesn't run the event loop. Probably you'll just have to start it and the code under test will then stop it on its own. You probably want to make the hard-coded timer interval configurable so the test doesn't waste 5 seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also make sense to check whether Worker exceeded RSS limit
is printed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yas, you are right... I've try to test it, but I guess I miss some knowledge on https://docs.mojolicious.org/Mojo/IOLoop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you've already found the documentation :-)
a323eda
to
0cd6cfe
Compare
0cd6cfe
to
6068cc9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6693 +/- ##
=======================================
Coverage 99.22% 99.23%
=======================================
Files 398 399 +1
Lines 40888 40895 +7
=======================================
+ Hits 40571 40581 +10
+ Misses 317 314 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
my $app = Mojolicious->new; | ||
$app->config->{global}{max_rss_limit} = 42; | ||
OpenQA::WebAPI::Plugin::MemoryLimit->register($app, undef); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mojo::IOLoop->start; | |
I guess you can set the conditions so the code under test will stop the loop automatically. And you should make the interval configurable to avoid wasting 5 seconds until that would happen.
Reference: https://progress.opensuse.org/issues/178948