Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

Delete "barback" from test config #87

Closed
wants to merge 1 commit into from
Closed

Delete "barback" from test config #87

wants to merge 1 commit into from

Conversation

whesse
Copy link

@whesse whesse commented Jun 30, 2016

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.

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
Copy link
Author

whesse commented Jun 30, 2016

@jakemac53 @nex3

@jakemac53
Copy link
Contributor

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
Copy link
Author

whesse commented Jun 30, 2016

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
Copy link
Contributor

jakemac53 commented Jun 30, 2016

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 dart-archive/package-bots#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
Copy link
Author

whesse commented Jun 30, 2016

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.

@whesse whesse closed this Jun 30, 2016
@whesse whesse deleted the whesse-patch-1 branch June 30, 2016 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants