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.

Sign up for GitHub

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

Jump to bottom

t: Extract helper for client user agent initialization #3414

Merged
merged 4 commits into from
Sep 29, 2020
Merged

t: Extract helper for client user agent initialization #3414

merged 4 commits into from
Sep 29, 2020

Conversation

okurz
Copy link
Member

@okurz okurz commented Sep 22, 2020

No description provided.

@okurz okurz force-pushed the enhance/test_client branch 2 times, most recently from 9dcd0bc to fd20452 Compare September 23, 2020 13:02
@codecov
Copy link

codecov bot commented Sep 24, 2020 •
edited
Loading

Codecov Report

Merging #3414 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3414      +/-   ##
==========================================
+ Coverage   95.54%   95.58%   +0.04%     
==========================================
  Files         362      363       +1     
  Lines       31506    31454      -52     
==========================================
- Hits        30101    30065      -36     
+ Misses       1405     1389      -16
Impacted Files Coverage Δ
t/09-job_clone.t 100.00% <ø> (ø)
t/23-amqp.t 100.00% <ø> (ø)
t/27-websockets.t 100.00% <ø> (ø)
t/31-client_archive.t 100.00% <ø> (ø)
t/32-openqa_client.t 100.00% <ø> (ø)
t/api/02-assets.t 95.18% <ø> (-0.28%) ⬇️
t/api/02-iso-download.t 97.48% <ø> (-0.05%) ⬇️
t/api/02-iso.t 99.20% <ø> (-0.01%) ⬇️
t/api/03-auth.t 100.00% <ø> (ø)
t/api/04-jobs.t 100.00% <ø> (ø)
... and 33 more

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 3a7368a...865bacb. Read the comment docs.

@okurz okurz force-pushed the enhance/test_client branch 3 times, most recently from 8a88249 to 67ea7d2 Compare September 24, 2020 06:47
@okurz
Tweak test timeouts for full-stack
91b6d29 
Increase circleCI timeout to be bigger than the sum of the number of
retries times the timeout for a single test run while still keeping the
timeout for a single test run higher than the test module internal
timeout.
@okurz
t: Simplify ui/17-product-log.t
8fd7d38
@okurz
t: Reuse existing schema variable in t/api/04-jobs.t
e54da58
@okurz
t: Extract helper for client user agent initialization
865bacb
@okurz okurz marked this pull request as ready for review September 25, 2020 16:54
kalikiana
kalikiana approved these changes Sep 29, 2020
View reviewed changes
Copy link
Member

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

The PR needs a better name. Like Unify use of OpenQA::Client with Test:Mojo. Otherwise it sounds like it's about tests for the client.

That said, I like it

@okurz okurz changed the title Enhance/test client t: Extract helper for client user agent initialization Sep 29, 2020
@okurz
Copy link
Member Author

okurz commented Sep 29, 2020

ok, updated the PR description to use the actual git commit subject message from the last commit because this one describes the most important, relevant change for this PR. The other changes are merely trivial things going along with it.

I have a specific question. Right now I named the new helper "client" but I wonder if the test code reads nicely with this name. Are you ok with the choice or have a better idea?

@kalikiana
Copy link
Member

ok, updated the PR description to use the actual git commit subject message from the last commit because this one describes the most important, relevant change for this PR. The other changes are merely trivial things going along with it.

I have a specific question. Right now I named the new helper "client" but I wonder if the test code reads nicely with this name. Are you ok with the choice or have a better idea?

I was pondering that but couldn't think of a better name. I think if we use it consistently as client($t) it's clear enough.

@okurz okurz merged commit 1bcb8a4 into os-autoinst:master Sep 29, 2020
@okurz okurz deleted the enhance/test_client branch September 29, 2020 14:16
Martchus
Martchus reviewed Sep 29, 2020
View reviewed changes
t/lib/OpenQA/Test/Client.pm

# setup test application with API access
# note: Test::Mojo looses its app when setting a new ua (see https://github.com/kraih/mojo/issues/598).
sub client {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have preferred adding this to the test utilities as it seems unlikely that related functions are added into this new module later. I would have also preferred a name like create_client for a function which actually creates a new instance and not just returns an existing one.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this is why this PR was open for 7 days and I asked for feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Martchus Martchus Martchus left review comments

@kalikiana kalikiana kalikiana approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@okurz @kalikiana @Martchus