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

fullstack: Make find_status_text wait for the element #4286

Conversation

kalikiana
Copy link
Member

Fixes the following error:

findElement: no such element: Unable to locate element: {"method":"css
selector","selector":"#info_box .card-body"}`

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

@kalikiana kalikiana force-pushed the full_stack_t_find_status_text_wait branch from d2d5d75 to c7e3a71 Compare October 8, 2021 17:43
@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #4286 (5dfff29) into master (89c0bdd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 5dfff29 differs from pull request most recent head 28328e6. Consider uploading reports for the commit 28328e6 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4286      +/-   ##
==========================================
- Coverage   97.94%   97.92%   -0.02%     
==========================================
  Files         371      371              
  Lines       33665    33614      -51     
==========================================
- Hits        32972    32917      -55     
- Misses        693      697       +4     
Impacted Files Coverage Δ
t/full-stack.t 96.62% <100.00%> (-0.35%) ⬇️
t/lib/OpenQA/Test/FullstackUtils.pm 96.80% <100.00%> (-3.20%) ⬇️
lib/OpenQA/Scheduler/Model/Jobs.pm 97.27% <0.00%> (-0.07%) ⬇️
lib/OpenQA/Schema/Result/Jobs.pm 98.49% <0.00%> (-0.01%) ⬇️
t/config.t 100.00% <0.00%> (ø)
t/03-auth.t 100.00% <0.00%> (ø)
t/28-logging.t 100.00% <0.00%> (ø)
t/43-cli-api.t 100.00% <0.00%> (ø)
... and 9 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 89c0bdd...28328e6. Read the comment docs.

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.

Please look into the decreased coverage. All files in t should definitely have 100% statement coverage

@kalikiana kalikiana force-pushed the full_stack_t_find_status_text_wait branch from c7e3a71 to 09d6fa0 Compare October 11, 2021 07:59
@perlpunk
Copy link
Contributor

The flaky coverage for FullstackUtils.pm is handled in
https://progress.opensuse.org/issues/99594

@perlpunk
Copy link
Contributor

The decreased coverage in t/full-stack.t is probably just due to the removal of a subroutine.

@perlpunk perlpunk requested a review from okurz October 12, 2021 13:59
@kalikiana
Copy link
Member Author

Maybe #4293 is also an alternative to this one.

@okurz
Copy link
Member

okurz commented Oct 13, 2021

The decreased coverage in t/full-stack.t is probably just due to the removal of a subroutine.

But the absolute coverage displayed as not 100% anymore. Do I again misinterpret the codecov numbers?

@kalikiana kalikiana force-pushed the full_stack_t_find_status_text_wait branch from 09d6fa0 to 7b0658d Compare October 13, 2021 13:19
t/lib/OpenQA/Test/FullstackUtils.pm Outdated Show resolved Hide resolved
@kalikiana kalikiana force-pushed the full_stack_t_find_status_text_wait branch 2 times, most recently from 4515b6d to 89c0314 Compare October 20, 2021 15:42
@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2021

This pull request is now in conflicts. Could you fix it? 🙏

@kalikiana kalikiana force-pushed the full_stack_t_find_status_text_wait branch from 89c0314 to 5dfff29 Compare October 22, 2021 08:53
@perlpunk
Copy link
Contributor

But the absolute coverage displayed as not 100% anymore. Do I again misinterpret the codecov numbers?

Looking into the Devel::Cover report:
https://77444-20883829-gh.circle-artifacts.com/0/cover_db/t-full-stack-t.html

There it shows which lines are uncovered. I don't see these lines at all in the codecov report :-/

@kalikiana kalikiana requested a review from okurz October 22, 2021 12:49
@kalikiana
Copy link
Member Author

Codecov seems to be happy now. Please only suggest further changes if you know what to change, though. I picked the line by trial-and-error.

@mergify
Copy link
Contributor

mergify bot commented Oct 25, 2021

This pull request is now in conflicts. Could you fix it? 🙏

@kalikiana kalikiana force-pushed the full_stack_t_find_status_text_wait branch from 5dfff29 to 64db3b7 Compare October 25, 2021 13:36
@okurz
Copy link
Member

okurz commented Oct 27, 2021

Overall the changes look like they do what the git commit message says but I am not convinced that we should do this waiting in all the places. I work prefer if we only do it where we actually understood that we need it. So I suggest to do the replacement only where tests actually fail and if we understood the reason in each case

@mergify
Copy link
Contributor

mergify bot commented Nov 4, 2021

This pull request is now in conflicts. Could you fix it? 🙏

@kalikiana
Copy link
Member Author

kalikiana commented Nov 8, 2021

Overall the changes look like they do what the git commit message says but I am not convinced that we should do this waiting in all the places. I work prefer if we only do it where we actually understood that we need it. So I suggest to do the replacement only where tests actually fail and if we understood the reason in each case

So after suggesting to rename find_status_text you're suggesting we check differently on a case-by-case basis? I guess I will kill the function and see what happens

Edit: This mean this will block on #4293

Fixes the following error:

    findElement: no such element: Unable to locate element: {"method":"css
    selector","selector":"#info_box .card-body"}`

See: https://progress.opensuse.org/issues/98952
@kalikiana kalikiana force-pushed the full_stack_t_find_status_text_wait branch from 64db3b7 to 28328e6 Compare November 8, 2021 14:08
@kalikiana kalikiana marked this pull request as draft November 16, 2021 19:18
@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2021

This pull request is now in conflicts. Could you fix it? 🙏

@stale
Copy link

stale bot commented Feb 23, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 23, 2022
@stale
Copy link

stale bot commented Mar 2, 2022

This Pull Request has been automatically closed as it did not have any activity in the last 97 days. Thank you.

@stale stale bot closed this Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants