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

Fix #257 : Added following test cases: #258

Closed
wants to merge 6 commits into from
Closed

Fix #257 : Added following test cases: #258

wants to merge 6 commits into from

Conversation

parauliya
Copy link
Contributor

@parauliya parauliya commented Aug 18, 2020 •
edited
Loading

  1. One workflow with one worker where all the actions are successful
  2. One workflow with one worker where an action gets failed
  3. One workflow with one worker where an action gets timed out
  4. Two workflows with one worker where all the actions are successful in both the workflows
  5. Two workflows with one worker where one of the workflow fails and other one runs successfully.
  6. Two workflows with one worker where one of the workflow timed out and other one runs successfully.

Description

This will enhance the end to end testing area.

Why is this needed

Fixes: #257

How Has This Been Tested?

Before adding any test case, I run it locally and then push in this PR.

How are existing users impacted? What migration steps/scripts do we need?

No impact.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests

@parauliya parauliya added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 18, 2020
@parauliya parauliya self-assigned this Aug 18, 2020
@parauliya parauliya added the do-not-merge Signal to Mergify to block merging of the PR. label Aug 18, 2020
@gianarb gianarb added the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Aug 18, 2020
@gianarb
Copy link
Contributor

gianarb commented Aug 18, 2020

Overall it looks good, I think the name of the functions should better explain what are you testing. Timeout?! Failure?! Too generic.

I am also happy to know if you find writing tests comfortable with the vagrant framework

@parauliya
Copy link
Contributor Author

@gianarb I have updated the function names. Please have a look.

@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Aug 18, 2020
@gianarb
Copy link
Contributor

gianarb commented Aug 18, 2020

@parauliya can you make the timeout to fail after 5s and not 10s please? Go has a strong timeout by default that starts after 10s and I think your test fails before the timeout takes action

@parauliya
Copy link
Contributor Author

@parauliya can you make the timeout to fail after 5s and not 10s please? Go has a strong timeout by default that starts after 10s and I think your test fails before the timeout takes action

@gianarb my timeout is 6s for the second action. First action complete succesfully and second action gets timeout after 6s.

@gianarb
Copy link
Contributor

gianarb commented Aug 20, 2020

Can you rebase and squash?! I think with the new timeout your tests should work in CI as well

@parauliya parauliya requested a review from gianarb August 21, 2020 07:57
@codecov
Copy link

codecov bot commented Aug 21, 2020 •
edited
Loading

Codecov Report

Merging #258 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #258   +/-   ##
=======================================
  Coverage   12.98%   12.98%           
=======================================
  Files           7        7           
  Lines        1186     1186           
=======================================
  Hits          154      154           
  Misses       1021     1021           
  Partials       11       11

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 1ba20b8...7cc4c52. Read the comment docs.

@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Aug 21, 2020
@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Aug 21, 2020
parauliya added 3 commits August 24, 2020 12:25
Fix tinkerbell#257 : Added following test cases:
8e067f5 
1. One workflow with one worker where all the actions are successful
2. One workflow with one worker where an action gets failed
3. One workflow with one worker where an action gets timed out

Signed-off-by: parauliya <[email protected]>
Incorporate review comments
f873690 
Signed-off-by: parauliya <[email protected]>
Added test cases for the scenario where there are two successfull wor…
c1862ba 
…kflows for one worker

Signed-off-by: parauliya <[email protected]>
parauliya added 3 commits August 24, 2020 12:31
Added test cases for the scenario where there are two workflows for o…
59ac6b0 
…ne worker.

Out of two workflows one fails and one runs successfully.

Signed-off-by: parauliya <[email protected]>
Added test case where one workflow is successful and one workflow is …
7187bab 
…timed out for same worker

Signed-off-by: parauliya <[email protected]>
Increase the test time from 5 sec to 10 secs
7cc4c52 
Signed-off-by: parauliya <[email protected]>
@parauliya parauliya removed the do-not-merge Signal to Mergify to block merging of the PR. label Aug 24, 2020
@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Aug 24, 2020
@gianarb
Copy link
Contributor

gianarb commented Sep 15, 2020

Currently exploring #277 .

@Raj-Dharwadkar
Copy link

I see conflicts here so please can you rebase to master?

@tstromberg
Copy link
Contributor

@parauliya - Thank you for your work on this. Do you mind if I close the PR until it's ready to be looked at again? It looks like it's doing the right thing.

Once you are able to make the changes, please re-open it and we'll make sure to review the PR in a more timely manner. I'm looking forward to it.

@tstromberg tstromberg closed this Jul 27, 2021
@mmlb mmlb mentioned this pull request May 5, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianarb gianarb Awaiting requested review from gianarb

Assignees

@parauliya parauliya

Labels
ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding more tests in vagrant
4 participants
@parauliya @gianarb @Raj-Dharwadkar @tstromberg