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

Add CLI feature to update quarkus app #1188

Merged
merged 3 commits into from
Jul 2, 2024
Merged

Conversation

mocenas
Copy link
Contributor

@mocenas mocenas commented Jul 1, 2024

Summary

We intend to test update of quarkus using CLI tool. Expose the update feature in the framework.

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@mocenas mocenas requested review from michalvavrik and gtroitsk July 1, 2024 12:43
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

it's ok for now. Please keep in mind that you will need WAY more and when you will open these follow-up PRs in FW for the update command, we will need tests. At least basic ones. Thank you

@mocenas
Copy link
Contributor Author

mocenas commented Jul 1, 2024

it's ok for now. Please keep in mind that you will need WAY more and when you will open these follow-up PRs in FW for the update command, we will need tests. At least basic ones. Thank you

RN I'm not really sure what will be required in FW, so I created just a basic update method, so we can start writing actual tests in the TS. Quarkus update command doesn't have many more CLI options for update and I'm not sure we will need them at all (but never say never).

Should we actually write tests for this in FW? As I see, quarkus-test-cli module has no tests at all, and those tests would probably duplicate what we (will) have in TS.

@gtroitsk
Copy link
Member

gtroitsk commented Jul 1, 2024

it's ok for now. Please keep in mind that you will need WAY more and when you will open these follow-up PRs in FW for the update command, we will need tests. At least basic ones. Thank you

RN I'm not really sure what will be required in FW, so I created just a basic update method, so we can start writing actual tests in the TS. Quarkus update command doesn't have many more CLI options for update and I'm not sure we will need them at all (but never say never).

Should we actually write tests for this in FW? As I see, quarkus-test-cli module has no tests at all, and those tests would probably duplicate what we (will) have in TS.

I think it will be good to have a simple example scenario for quarkus app update in FW. Some tests for CLI are in QuarkusCliClientIT.java.

@michalvavrik
Copy link
Member

Should we actually write tests for this in FW?

If you make changes to FW that you cannot verify with FW CI, we cannot know whether changes to FW are correct without running TS which is not automated with FW changes. IMO the FW tests should verify functionality of FW while TS tests should verify Quarkus.

That said, sometimes it is not feasible to test every PR and not all my PRs have tests. I've already approved.

As I see, quarkus-test-cli module has no tests at all

Here are tests https://github.com/quarkus-qe/quarkus-test-framework/tree/main/examples/quarkus-cli.

and those tests would probably duplicate what we (will) have in TS.

In TS you will want to verify relevant Quarkus release migrations, here you just want to verify that what your code says it does. It is different because you don't care about specific changes but about specific functionality. I cannot tell you whether there will be duplication because I don't know your test coverage :-) So I don't mind little duplication.

@michalvavrik
Copy link
Member

michalvavrik commented Jul 2, 2024

we can start writing actual tests in the TS

What I meant was that this PR is not sufficient to submit tests in the TS and you don't need FW release to start writing tests in the TS with snapshot FW. This PR is completely fine, thank you.

@mocenas
Copy link
Contributor Author

mocenas commented Jul 2, 2024

I've added a test for update feature. Sorry it didn't dawn on me, that "examples" dir is containing actual tests for the framework.

@mocenas mocenas requested a review from michalvavrik July 2, 2024 07:42
Copy link
Member

@michalvavrik michalvavrik 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 test.

@michalvavrik
Copy link
Member

michalvavrik commented Jul 2, 2024

I've added a test for update feature. Sorry it didn't dawn on me, that "examples" dir is containing actual tests for the framework.

I dropped tests from Quarkus CLI module just a month ago #1164 :-D So you were not that wrong.

@mocenas mocenas merged commit d996d08 into quarkus-qe:main Jul 2, 2024
8 checks passed
@mocenas mocenas deleted the cli_update branch July 2, 2024 10:53
jcarranzan pushed a commit to jcarranzan/quarkus-test-framework that referenced this pull request Aug 5, 2024
* Add CLI feature to update quarkus app
@mocenas mocenas mentioned this pull request Aug 14, 2024
11 tasks
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.

3 participants