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

Create run.yaml for tmt try #2889

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Create run.yaml for tmt try #2889

merged 1 commit into from
Jul 31, 2024

Conversation

skycastlelily
Copy link
Collaborator

@skycastlelily skycastlelily commented Apr 25, 2024

Pull Request Checklist

Resolves #2888

  • implement the feature
  • extend the test coverage

@skycastlelily
Copy link
Collaborator Author

skycastlelily commented Apr 25, 2024

fix this issue:#2888 ^^

@lukaszachy
Copy link
Collaborator

/packit test

@lukaszachy lukaszachy added this to the 1.34 milestone Apr 30, 2024
@lukaszachy lukaszachy added the command | try tmt try command label Apr 30, 2024
@happz happz linked an issue May 17, 2024 that may be closed by this pull request
@happz happz changed the title Create run.yaml for try Create run.yaml for tmt try Jun 11, 2024
Copy link
Collaborator

@thrix thrix left a comment

Choose a reason for hiding this comment

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

LGTM

@psss psss changed the title Create run.yaml for tmt try Create run.yaml for tmt try Jun 11, 2024
@happz happz added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Jun 12, 2024
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Overall looks good. I gave it a quick try and ran into regression:

cd
tmt try

Before I was able to enter directory without tmt metadata and quickly start experimenting but now I get the following:

No metadata found in the '.' directory. Use 'tmt init' to get started.

Could you please look into it? Also, what about extending the related test coverage a bit? We could extend the keep test case here:

rlPhaseStartTest "Keep"
rlRun -s "./keep.exp" 0 "Quit the session"
rlAssertGrep "Run .* kept unfinished. See you soon!" $rlRun_LOG
rlPhaseEnd

It would be also good to add a test case for the above-mentioned scenario which was affected by the change.

@happz
Copy link
Collaborator

happz commented Jun 12, 2024

Deferring to 1.35 to get more time to resolve the (minor yet pesky) issues reported in #2889 (review)

@skycastlelily
Copy link
Collaborator Author

We could extend the keep test case here:

rlPhaseStartTest "Keep"
rlRun -s "./keep.exp" 0 "Quit the session"
rlAssertGrep "Run .* kept unfinished. See you soon!" $rlRun_LOG
rlPhaseEnd

It would be also good to add a test case for the above-mentioned scenario which was affected by the change.

Looks like keep test case also covers that scenario

(dev) lnie@fedora:~/tmt$ cat ./tests/try/basic/data/keep.exp
#!/usr/bin/expect -f
# Select a plan and keep the run

spawn tmt try -p plan
expect "What do we do next?"
send -- "k\r"
expect eof

I can reproduce that issue with my original MR but can't produce it after updated the code

[lnie@ceph-04 ~]$ tmt try -p plan
/var/tmp/tmt/run-009
No metadata found in the '.' directory. Use 'tmt init' to get started.

[lnie@ceph-04 ~]$ tmt try -p plan
/var/tmp/tmt/run-010
No plan matching 'plan' found.

Please let me know if I miss anything,thanks:)

@happz happz removed the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Jun 20, 2024
@happz happz requested a review from psss June 20, 2024 14:01
@therazix
Copy link
Collaborator

therazix commented Jul 8, 2024

@skycastlelily It looks like commit 421fea8 doesn't belong to this pull request and shouldn't be here.

@skycastlelily
Copy link
Collaborator Author

skycastlelily commented Jul 10, 2024 via email

@happz
Copy link
Collaborator

happz commented Jul 31, 2024

@skycastlelily would you be able to add a test as suggested by Petr in #2889 (review)? Even a trivial one would be better than nothing.

Nevermind, I was shown #2889 (comment)

@thrix
Copy link
Collaborator

thrix commented Jul 31, 2024

@skycastlelily would you be able to add a test as suggested by Petr in #2889 (review)? Even a trivial one would be better than nothing.

I believe she covered this in this reply #2889 (comment)

@happz happz added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Jul 31, 2024
@happz happz added the ci | full test Pull request is ready for the full test execution label Jul 31, 2024
@happz
Copy link
Collaborator

happz commented Jul 31, 2024

/packit test

@happz
Copy link
Collaborator

happz commented Jul 31, 2024

Unrelated failures, merging.

@happz happz merged commit 67e2006 into main Jul 31, 2024
19 of 20 checks passed
@happz happz deleted the save-try branch July 31, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution command | try tmt try command status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to delete guests created by tmt try
8 participants