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

test: remove dependency to html-template in integration tests #1202

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

derberg
Copy link
Member

@derberg derberg commented Apr 25, 2024

The issue is that all our integration tests depended on html-template

It was fine just until this week when we did generator v2 release - and now our tests will not work until html-template actually starts supporting v2 - it is because we have a functionality that templates can define what generator versions they depend on -> https://github.com/asyncapi/java-template/blob/master/package.json#L102

This PR:

  • removes html-template from tests
  • introduces new local templates, simplistic for react and nunjucks - and now these are used in tests
  • local verdaccio (npm registry) is extended to work with all integration tests (not registry tests only) so we have a place where we can during tests publish the local templates (for real production testing)
  • eslint had to be reconfigured a bit due to introduction of react template in the code - so we can lint it properly

Side effect: we have local test templates that along the way we can extend with more features and extend integration tests to test more functionality.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@derberg
Copy link
Member Author

derberg commented Apr 25, 2024

@Florence-Njeri I updated readme in test folder so you need to approve as well

@lmgyuan
Copy link
Collaborator

lmgyuan commented Apr 25, 2024

I understand why an alternative simplistic test template is needed to replace HTML template. But my concern is that will using a local template require maintainers always to update it to avoid situations where the tests pass but the generator is not working in the actual usage? If this is temporary, and we will update the HTML template and others to support the generator v2 release, then I think using a local simplistic template for now is understandable. : ) @derberg

@derberg
Copy link
Member Author

derberg commented Apr 25, 2024

@lmgyuan the reality is that we did not use html-template in a way that there were some great features to test. So local templates give us more flexibility actually, more can be tested long term.

Do why we didn't do it from the beginning? I added first integration tests when there was no use case for testing installation from nom registry. So there was no use case to figure there is something like verdaccio. So I neglected local templates as templates that do not enable us to test all scenarios. Now with verdaccio it is super easy and we can test basically any scenerio.

/rtm

@asyncapi-bot asyncapi-bot merged commit a688187 into asyncapi:master Apr 25, 2024
12 checks passed
@lmgyuan
Copy link
Collaborator

lmgyuan commented Apr 26, 2024

@lmgyuan the reality is that we did not use html-template in a way that there were some great features to test. So local templates give us more flexibility actually, more can be tested long term.

Do why we didn't do it from the beginning? I added first integration tests when there was no use case for testing installation from nom registry. So there was no use case to figure there is something like verdaccio. So I neglected local templates as templates that do not enable us to test all scenarios. Now with verdaccio it is super easy and we can test basically any scenerio.

/rtm

This makes a lot of sense! Thanks for your explanation!

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants