dart-lang / test

A library for writing unit tests in Dart.
https://pub.dev/packages/test
495 stars 214 forks source link

Add support for multiple full paths on macos #2276

Closed natebosch closed 2 months ago

natebosch commented 2 months ago

We cannot look up multiple basename commands in the system path and the current macOsExecutable configuration may have existing uses in dart_test.yaml files so it isn't safe to require full paths. Add a separate macOsAbsolutePaths configuration to enable internal definitions that check multiple full paths and execute the first one that exists.

Add the basename firefox as a fallback. Adding that command to the path is one workaround for users with firefox installed in an unexpected location. This is also the only approach that is compatible with mac on GitHub actions using the setup-firefox action.

There is no support here for dart_test.yaml. Users that are configuring an executable for macOS will continue to have support for only a single value, even if they are specifying an absolute path.

Tests remain skipped because our mono repo setup does not have a way to include the setup-firefox action.

github-actions[bot] commented 2 months ago

PR Health

Changelog Entry :heavy_check_mark: | Package | Changed Files | | :--- | :--- | Changes to files need to be [accounted for](https://github.com/dart-lang/ecosystem/wiki/Changelog) in their respective changelogs.
Package publish validation :heavy_check_mark: | Package | Version | Status | | :--- | ---: | :--- | | package:checks | 0.3.1-wip | WIP (no publish necessary) | | package:test | 1.25.9-wip | WIP (no publish necessary) | | package:test_api | 0.7.4-wip | WIP (no publish necessary) | | package:test_core | 0.6.6-wip | WIP (no publish necessary) | Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.
natebosch commented 2 months ago

This is the least invasive way I could see to make this change. I'm not sure if it's worth landing, but opening the PR for discussion.

natebosch commented 2 months ago

I think if we wanted we could support this in dart_test.yaml. We could accept either a single string, or a list of strings, and we could validate when parsing that only full paths are used if it is a list. We already have some inconsistency between the meaning of the arguments across OSes, so I don't think it's a particular problem to add this option/caveat exclusive to macOS.

natebosch commented 2 months ago

This fixes the problem for me on a local mac. It looks like maybe firefox doesn't come by default on github actions macos runs? I don't think mono package supports adding a firefox setup step. Any suggestions on a workaround @kevmoo ?

natebosch commented 2 months ago

It looks like maybe firefox doesn't come by default on github actions macos runs?

No, it still fails even if I manually add the setup-firefox steps.

natebosch commented 2 months ago

I cannot figure out the mac problem. Landing this is still worth it because it does fix things for me locally. @jakemac53 any concerns about landing this without unskipping the tests on CI, or with using the basename as a fallback?

jakemac53 commented 2 months ago

I cannot figure out the mac problem. Landing this is still worth it because it does fix things for me locally. @jakemac53 any concerns about landing this without unskipping the tests on CI, or with using the basename as a fallback?

I am fine with it, we don't have a lot of people asking about this, so it isn't worth dumping a ton of time into. If people do get broken again they will file issues (plus, there is a workaround to set your own path).