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

matcher: should be their own pkg #2331

Closed
DartBot opened this issue Jun 5, 2015 · 13 comments
Closed

matcher: should be their own pkg #2331

DartBot opened this issue Jun 5, 2015 · 13 comments
Labels
package:matcher type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@DartBot
Copy link

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/1081711?v=3" align="left" width="96" height="96"hspace="10"> Issue by jmesserly
Originally opened as dart-lang/sdk#10238


I don't think matchers depend on unittest. Presumably they should not be in the unittest pkg?

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2762632?v=3" align="left" width="48" height="48"hspace="10"> Comment by gramster


I felt that way too but I think there was some pushback. If nothing else separating them out would mean they actually show up in API docs!

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/480590?v=3" align="left" width="48" height="48"hspace="10"> Comment by Ladicek


Issue #142 is of historical significance here.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/46275?v=3" align="left" width="48" height="48"hspace="10"> Comment by munificent


I'm fine with splitting them up. I think at the time our package reuse story wasn't quite as mature. We were still using SDK packages and the version solver wasn't as smart. Today, it should be easier to split these up while still being easy for users to consume them.

Would unittest.dart will export matchers.dart, or would users have to import both in their tests?

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2049220?v=3" align="left" width="48" height="48"hspace="10"> Comment by sigmundch


I'd be happy to split them up, but I'd prefer if unittest reexports them by default.

@DartBot DartBot added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Jun 5, 2015
@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2762632?v=3" align="left" width="48" height="48"hspace="10"> Comment by gramster


My assumption was that if we split them up, unittest would not import matcher, and so users would import both if they wanted to use out matchers and expect(). OTOH they could use a different matcher library if they want.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2762632?v=3" align="left" width="48" height="48"hspace="10"> Comment by gramster


We do have a different issue, which is asking for unittest to be renamed unit_test to be consistent with the style guide. So an option - although it's kinda hokey - would be to move unittest to unit_test, and have unittest be a shell that imports and reexports unit_test and matcher. That would mean we don't break any existing tests.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2762632?v=3" align="left" width="48" height="48"hspace="10"> Comment by gramster


The renaming issue is https://code.google.com/p/dart/issues/detail?id=10120

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2049220?v=3" align="left" width="48" height="48"hspace="10"> Comment by sigmundch


Not sure.

I'd do the unittest exports unit_test + matchers just as a transition, maybe include a deprecation message so that users start switching their imports.

If people already have to rewrite their imports because we rename the package to 'unit_test', then that would be a great point to do a few other things like:

  • introduce new entrypoint files for each configuration (so instead of import unittest + import vm_config, you just import 'package:unit_test/vm_testing.dart')
  • maybe not including 'matchers' by default

I personally prefer that matchers is included because the great majority out there just wants to use it together with unittest, so there is less set up to do. If we don't it with unittest.dart, do people need to also add a pub dependency to get matchers?

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/46275?v=3" align="left" width="48" height="48"hspace="10"> Comment by munificent


  • introduce new entrypoint files for each configuration (so instead of import unittest + import vm_config, you just import 'package:unit_test/vm_testing.dart')

I'm hesitant to do this because I worry it means people will couple their unit tests to a single configuration even when the code under test can run on multiple configs.

  • maybe not including 'matchers' by default

I believe unittest is and will be coupled to matcher, it's just that matcher is not coupled to unittest.

If we don't it with unittest.dart, do people need to also add a pub dependency to get matchers?

No, unittest depends on it, so you will get it transitively automatically.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/2762632?v=3" align="left" width="48" height="48"hspace="10"> Comment by gramster


Removed the owner.
Added Triaged label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/444270?v=3" align="left" width="48" height="48"hspace="10"> Comment by seaneagan


Any updates on this? I have some use cases for matchers outside of unit testing, and it's weird to have to include unittest to get to them. It also seems error prone, as unittest is often a dev_dependency, so it would work in development, but fail in production when unittest no longer appears in the "packages" dir.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/17034?v=3" align="left" width="48" height="48"hspace="10"> Comment by kevmoo


Added Pkg-matcher label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/17034?v=3" align="left" width="48" height="48"hspace="10"> Comment by kevmoo


matcher has been its own package for a month+ now

Closing now that unittest no longer exports it: dart-lang/sdk@d60ad1a


Added Fixed label.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:matcher type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

2 participants