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

collector.collect() doesn't throw in 1.1.0 #36

Open
markreid opened this issue Feb 13, 2018 · 2 comments
Open

collector.collect() doesn't throw in 1.1.0 #36

markreid opened this issue Feb 13, 2018 · 2 comments

Comments

@markreid
Copy link

Hi,

We've just updated Publication Collector to 1.1.0 and it's broken a few of our tests. Given that you've added support for Promises, I'd say this is probably something you'd consider "expected behaviour", but I was thinking you might want to update the changelog/history to note that it's a potentially breaking change.

Example of the problem:

// FooBarQux is a publication that has a check(argument, String) call for type-checking arguments
describe('FooBarQux subscription', () => {
    it('Takes a String argument', () => {
      const collector = new PublicationCollector();
      expect(() => collector.collect('OrganisationDetail'))
      .to.throw(/Match error/);

      expect(() => collector.collect('FooBarQux', {
        foo: 'bar',
      }))
      .to.throw(/Match error/);
    });
  });

This test has regressed because collector.collect() no longer re-throws errors that are thrown by the publication; instead it returns a promise and rejects it.

This is obviously expected behaviour and we've updated our test suite accordingly, but I can only assume that anybody else testing for publication errors has been doing something similar, so I'd argue that this is potentially a breaking change?

@johanbrook
Copy link
Collaborator

Hi @markreid. Thanks for reporting this in, and apologies for not noting this more clearly in the README. I'll fix that.

I believe the Meteor version resolver tool is quite conservative when it comes to updating packages, so most people will probably still be on existing versions of this package unless explicitly updated. If people don't wanna update their test code, they can still use an older version of this package.

Thanks again!

johanbrook added a commit that referenced this issue Feb 14, 2018
@jankapunkt
Copy link
Member

jankapunkt commented Feb 16, 2018

Hi I ran over the same issue (I usually update all packages when there is a breaking change in the meteor core as in 1.6.1)

It broke all my tests that expected to throw on invalid parameters. For me it would be important to know why this is the new behavior (rejecting the promise) and how I can adapt my tests to that, so that I also get rid of the warnings:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2):#

Edit:

Just re-checked your REAMDE again, got it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants