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

Fix cloud test progress status in CLI #1490

Merged
merged 11 commits into from
Jul 6, 2020
Merged

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Jun 8, 2020

Closes #1488

It's still a bit janky, but at least it shows the status text in the same place as before. Please test it out locally and let me know.

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020

Codecov Report

Merging #1490 into new-schedulers will decrease coverage by 0.17%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           new-schedulers    #1490      +/-   ##
==================================================
- Coverage           77.33%   77.15%   -0.18%     
==================================================
  Files                 162      162              
  Lines               13127    13155      +28     
==================================================
- Hits                10152    10150       -2     
- Misses               2455     2485      +30     
  Partials              520      520              
Impacted Files Coverage Δ
cmd/cloud.go 8.10% <0.00%> (-0.92%) ⬇️
cmd/run.go 9.82% <0.00%> (+0.73%) ⬆️
cmd/ui.go 25.88% <0.00%> (-5.78%) ⬇️
lib/executor/vu_handle.go 93.69% <0.00%> (-1.81%) ⬇️

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 41a4f73...f57e189. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

Hmm to be honest, I'm not sure if I like the shifting text on the left 😕 Maybe we can have 2/3 fixed-width labels on the left, Init / Run / Done, and keep showing the detailed run status text on the right? cc @mpandurovic

@imiric
Copy link
Contributor Author

imiric commented Jun 9, 2020

@na-- Yeah, that might make more sense to bring it in line with the k6 run progressbars. See 66b0ad7.

I added the elapsed/total time while the test is running, since showing Run [==>--] Running is not that useful. It's far from perfect, as it doesn't take into account the gracefulStop/gracefulRampDown, doesn't support stages right now, and the timings are way off because of the 2s ticker, but at least it shows an approximation.

Here's what it looks like on 66b0ad7a. Let me know if this should be refined or if we should just hide "Running" on the right side and not bother with the time.

EDIT: Pushed a fix in 5d27e4b to hopefully address all duration scenarios.

@imiric imiric force-pushed the fix/1488-cli-cloud-status branch from 6dab874 to 5d27e4b Compare June 9, 2020 10:36
@imiric imiric requested a review from na-- June 9, 2020 10:49
cmd/cloud.go Outdated Show resolved Hide resolved
@imiric imiric force-pushed the fix/1488-cli-cloud-status branch 2 times, most recently from 05a4528 to e931fc2 Compare June 16, 2020 09:22
@mstoykov mstoykov added this to the v0.27.0 milestone Jun 30, 2020
cmd/cloud.go Show resolved Hide resolved
cmd/cloud.go Outdated Show resolved Hide resolved
cmd/cloud.go Outdated Show resolved Hide resolved
core/local/local.go Outdated Show resolved Hide resolved
core/local/local.go Outdated Show resolved Hide resolved
lib/executor/ramping_vus.go Outdated Show resolved Hide resolved
@imiric imiric force-pushed the fix/1488-cli-cloud-status branch from 6df9b4b to cc01c16 Compare July 6, 2020 10:29
@imiric imiric requested review from na-- and mstoykov July 6, 2020 10:47
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, though I don't like the extra GetFullExecutionRequirements() calculation in the case of k6 run... That can be very costly for large test runs, especially if we have a ramping-vus executor... We'd just be adding one more layer to #1499 / #1427...

So, despite printExecutionDescription() having a ton of parameters, I think we should add one more for now, and pass the already pre-calculated execScheduler.GetExecutionPlan() in k6 run, so we don't have another performance degradation. But we should also make a note in #1499 and/or #1427 to clean it up when we refactor the APIs and clean up the other issues.

@imiric imiric merged commit c5e62ff into new-schedulers Jul 6, 2020
@imiric imiric deleted the fix/1488-cli-cloud-status branch July 6, 2020 13:22
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.

4 participants