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

Expect unawaited futures in test.expect (attempt to fix #419) #430

Closed
wants to merge 1 commit into from

Conversation

ochafik
Copy link
Contributor

@ochafik ochafik commented Feb 19, 2017

Note: I couldn't find an easy way to test the package origin from the tests themselves, suggestions welcome! Also, not sure if that will work with summaries (do we still have sourcefiles then?), etc... Pointers as to how to test would be welcome!

Copy link
Member

@bwilkerson bwilkerson left a comment

Choose a reason for hiding this comment

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

I think this will work. Summaries record where the data came from, so they are able to re-create the Source associated with a library.

@zoechi
Copy link

zoechi commented Feb 19, 2017

An alternative would be #419 (comment) to allow APIs themselves to opt out instead of special-casing in the linter rule.

@pq
Copy link
Member

pq commented Feb 19, 2017

An alternative would be #419 (comment) to allow APIs themselves to opt out instead of special-casing in the linter rule.

@zoechi 👍 --- this is something we're actively discussing. I think this is a good step in the meantime though.

Thanks @ochafik !

Copy link
Member

@pq pq left a comment

Choose a reason for hiding this comment

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

Nit...

if (libUri == null) return false;

return libUri.startsWith('package:test/') ||
libUri.startsWith('package:unittest/');
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the unittest check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(not that it matters anymore, but fyi most our tests are still using unittest :-))

Copy link
Member

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

Let's hold off on this. I have some open discussions on the recent change to pkg/test.

@pq
Copy link
Member

pq commented Feb 19, 2017

@kevmoo : is that conversation on an issue somewhere? If not, could you post status somewhere we can follow?

@pq
Copy link
Member

pq commented Feb 22, 2017

Thanks @kevmoo .

I think as of dart-lang/test#546, this becomes moot and @ochafik you can safely close this?

@ochafik
Copy link
Contributor Author

ochafik commented Feb 22, 2017

Cool, closing this, thanks!

@ochafik ochafik closed this Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants