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

feat(nextcloud): test validity of the generated clients #1014

Closed
wants to merge 2 commits into from

Conversation

Leptopoda
Copy link
Member

@Leptopoda Leptopoda commented Oct 23, 2023

implements: #555

@Leptopoda Leptopoda force-pushed the tool/nextcloud/check_generated_code branch from da9499b to be66a5a Compare October 25, 2023 13:24
@Leptopoda
Copy link
Member Author

depends on #1024
Looks like the CI liked my changes.

@Leptopoda Leptopoda force-pushed the tool/nextcloud/check_generated_code branch from be66a5a to 15ed36e Compare October 28, 2023 08:04
@Leptopoda
Copy link
Member Author

This is based on the work we did in #1035

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

This looks mostly good, but I still don't like that it gets so complex.
I have an idea how we can do this better:

Move the ensure_validity_test.dart files into a source_verification_test/ dir inside the packages.
Then adjust the melos commands to run with concurrency 1 for packages with that dir and point the test command only to that dir.
Of course this makes it a bit more work to run those tests locally for a single package because you now need to run fvm dart test source_verification_test/, but I would argue that you rarely want to run those tests locally if not never.
They are really just for the CI to ensure everything committed is up-to-date.

Does this make sense? Maybe I'm still missing something, but in my head this is much better than the tricks with the tags and checking for the specific test file name.

@provokateurin
Copy link
Member

The other option that gets completely rid of these problems and the build_verify way is to just setup a workflow that runs the generate scripts and checks for diffs with git diff. That would be a more classic approach which might be better because we just avoid this whole mess.

@Leptopoda
Copy link
Member Author

The other option that gets completely rid of these problems and the build_verify way is to just setup a workflow that runs the generate scripts and checks for diffs with git diff. That would be a more classic approach which might be better because we just avoid this whole mess.

Feel free to implement this. I'm not a script wizard as you are :)
I think this approach is completely fine and once we support more platforms we might want to expand the tags even further (thinking about web tests).

Maybe we can just skip the tag entirely in packages that don't use it but I'm not sure this would work.

@provokateurin
Copy link
Member

Ok I'll do that and open a new PR

@Leptopoda Leptopoda closed this Oct 29, 2023
@Leptopoda Leptopoda deleted the tool/nextcloud/check_generated_code branch October 29, 2023 08:43
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.

2 participants