migrate to dart test configuration

[Leptopoda]

extracted from #1014
Needed for #1024

[Leptopoda]

This was really painful.
flutter test -t source_verification exits with 1 if no tests match the given tag. Therefore the migration of the nc package to the dart test configuration is coming with #1014

[provokateurin]

Seems like it is still broken.
Maybe a stupid question, but can't you just specify the concurrency for the source_verification tag and skip the mess with the commands?

[Leptopoda]

1. we need two separate commands to run all normal tests with a higher concurrency
2. we need the tags to group the tests
3. We need some other hacks to get the tags working because as mentioned before specifying a tag to execute will exit with an exit code of 1 if no tests match the specified tags
4. the dart_test.yaml files are not strictly needed but I thought they make it a bit simpler to maintain.

Does that make sense? The current issue is that the timeout overrides per tag are not taken into account so our scale factor of 20 that we use to get 10minutes isn't used.
Do you have any better idea to achieve this?

[Leptopoda]

When I tested it yesterday the retry count was correctly applied to tests with the integration tag.

[provokateurin]

Can you point me to the docs for this config file? Maybe there is just something wrong with our config

[Leptopoda]

https://github.com/dart-lang/test/blob/master/pkgs/test/doc/configuration.md

[provokateurin]

As far as I understand the docs the timeout you configured should work just fine. It also allows you to specify the concurrency for a tag, but I assume that would only apply to tests that have this tag (which we don't want)?

[Leptopoda]

It also allows you to specify the concurrency for a tag

This also only allows us to set a fixed concurrency but we want it to be dynamic through nproc.

but I assume that would only apply to tests that have this tag

I think you could also specify it globally for all test, similar to the global timeout of 30s

[provokateurin]

This also only allows us to set a fixed concurrency but we want it to be dynamic through nproc.

Yes, but I would force that through the command line and then override it for that one tag to be 1.

I think you could also specify it globally for all test, similar to the global timeout of 30s

Sure, but then we run all tests with concurrency 1 which is bad

[Leptopoda]

But I don't think the concurrency discussion helps us in any way with the timeout problems :)

[Leptopoda]

Ok leaving the base timeout out fixed it.

Can you please review @provokateurin

[provokateurin]

Can you report that in the dart test repo? I don't think this is intended

[Leptopoda]

This might be intended.

Unlike test configuration, runner configuration affects the test runner as a whole rather than individual tests. It can only be used at the top level of the configuration file.

and our dart_test_base.yaml is such a runner configuration

[Leptopoda]

Can you please approve so we can finally land the fix on dynamite.

[provokateurin]

Tbh I still don't like this because I can't just run fvm dart test in the nextcloud package anymore.

[Leptopoda]

can you elaborate?
both fvm dart test, melos run test and through my IDE work as expected on the latest version of this branch.

(I just noticed that the melos command also excludes "integration" so I'll adjust that)

[provokateurin]

Ok so this works, but we don't really need the dart test configuration at all. Just set the timeout and tag for that one test and keep the changes in melos.yaml and .github

[provokateurin]

Instead of filtering on the dart_test.yaml file which could very well be used for other reasons please use something else. Maybe just a file called source_verification.txt with a comment inside to explain why it is needed.

[Leptopoda]

Ok so this works, but we don't really need the dart test configuration at all.

I disagree. With #1014 this would get duplicated and even more if we decide to enable something similar on the neon packages (once we use go_router we'll also generate a lot of code there).

Instead of filtering on the dart_test.yaml file which could very well be used for other reasons please use something else. Maybe just a file called source_verification.txt with a comment inside to explain why it is needed.

In this case I'd rather keep the test name the same and filter based on that.

[provokateurin]

I disagree. With #1014 this would get duplicated and even more if we decide to enable something similar on the neon packages (once we use go_router we'll also generate a lot of code there).

I see, but I would have expected the shared config file in the PR where you actually have this problem and not in a different PR if that makes sense?

In this case I'd rather keep the test name the same and filter based on that.

I thought the problem with filtering for a tag or test name is that the command fails if there are no tests matching the filter? If not then please go for it

[Leptopoda]

I see, but I would have expected the shared config file in the PR where you actually have this problem and not in a different PR if that makes sense?

This is a chicken and egg problem. I initially only wanted to add the validity testing for the nc package which can't go through because of the dynamite header bug which can't go through because of the dynamite_e2e validity test timeout.

If you want I can also just dirty fix the timeout with this PR by hard coding a longer one into the one test and do all the shared dart_test.yaml config stuff after all the blocking PRs went through.
Is that better? Sorry for the mess but I don't know what way is preferred.

I thought the problem with filtering for a tag or test name is that the command fails if there are no tests matching the filter? If not then please go for it

We cant do that on the dart test side but I meant the filter for the melos command wrapping it.
It could become something like: (untested)

melos exec --scope="$packages_glob" --concurrency=1 --fail-fast --file-exists=test/ensure_validity_test.dart -- "flutter test --concurrency=1 -t source_verification"