search

dart-archive / observe

Support for marking objects as observable, and getting notifications when those objects are mutated
https://pub.dartlang.org/packages/observe
BSD 3-Clause "New" or "Revised" License
13 stars 6 forks source link

Delete "barback" from test config #87

Closed whesse closed 8 years ago

whesse commented 8 years ago

This option seems to be wrong, and is breaking the testing on the package bots.

Currently, they are running "pub build test" and "pub run test build/test", and reporting all sorts of errors on all platforms.

If we do a clean checkout of observe, and run "pub get" "pub build test" then: pub run test build/test reports "ran 0 tests" on all platforms (vm, chrome, firefox, dartium) and pub run test or pub run test test run all the tests, and they all pass, on all platforms. On platforms like chrome and firefox, the command compiles the tests to a temporary directory and runs them from there.

Therefore, I think that omitting the "barback" setting, and therefore running "pub run test" instead of "pub run test build/test" is the right way to test these.

whesse commented 8 years ago

@jakemac53 @nex3

jakemac53 commented 8 years ago

Afaik we do want to run these tests before and after transformation to ensure everything works in all modes.

If you run pub serve test and then pub run test -p dartium --pub-serve=8080 it does run tests. I don't know how that compares to running pub build test and then pub run test build/test.

whesse commented 8 years ago

Well, let's file an issue to verify the after-transformation runs, but I believe that this is what just 'pub run test -p chrome' is also doing - it starts up a server, compiles the tests in the same way, and serves them to the browser just as when the --pub-serve option is included.

In any case, running with the build/test directory is incorrect, and does nothing, or fails weirdly, and running with test directory seems to do exactly what we want. This is also what all packages without the barback option in their config are doing.

So I think removing the barback option is the right thing to do here, and an expert in how the test package works can say if there is some other way than just 'pub run test -p [browser]' that is needed to do all of the building and testing.

jakemac53 commented 8 years ago

Well, let's file an issue to verify the after-transformation runs, but I believe that this is what just 'pub run test -p chrome' is also doing - it starts up a server, compiles the tests in the same way, and serves them to the browser just as when the --pub-serve option is included.

I don't think that is the case, it will run dart2js but not the transformers afaik.

In any case, running with the build/test directory is incorrect, and does nothing, or fails weirdly, and running with test directory seems to do exactly what we want. This is also what all packages without the barback option in their config are doing.

The supported way of running the transformed tests is using pub serve and then passing the --pub-serve=${port} flag to pub run test (see docs here). Also see the related issue https://github.com/dart-lang/package-bots/issues/6.

So I think removing the barback option is the right thing to do here, and an expert in how the test package works can say if there is some other way than just 'pub run test -p [browser]' that is needed to do all of the building and testing.

I think the proper fix is to change how the bots run tests. It seems like maybe pub build test pub run test build/test option used to work in the past, but no longer works in some more recent version of test?

whesse commented 8 years ago

Yes, I agree, this option can stay, but needs to trigger runs with 'run test --pub-serve ...', not with 'run test ... build/test'. Closing this pull request.