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

Fix CI: update yeoman-generator to 3.x, apply latest prettier formatting #1578

Merged
merged 2 commits into from
Jul 31, 2018

Conversation

virkt25
Copy link
Contributor

@virkt25 virkt25 commented Jul 30, 2018

Make master go green again by:

Commit 1

  • Updating yeoman-generator to 3.x
    • Updating CLI tests as needed

Commit 2

  • Applying latest prettier formatting standards
    • Empty properties in front-matter have been removed.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@shimks
Copy link
Contributor

shimks commented Jul 31, 2018

For context, the tests are failing because of the upgrades made between yeoman-generator 2 -> 3. The changes are listed here https://github.com/yeoman/generator/blob/master/lib/index.js#L433, where the error being thrown is emitted AND rejected. In v2, the error was emitted, and then a callback was called with the error. The problem is that in the test helper https://github.com/yeoman/yeoman-test/blob/master/lib/run-context.js#L121, the rejection is emitted AGAIN here.

I've spent hours into trying to fix around the code in yeoman so that the error can just be rejected, but despite being caught in a catch block, it will always show up as an unhandled rejected promise. Alternatively, just emitting the error in favor of rejecting it will make the tests pass as is.

If anybody can see something that I missed that can be fixed in yeoman to address this issue, I'd be super happy know so that I feel my time investment in this issue can be justified :p

I'm just very peeved at yeoman that we have to essentially write our own error-handling system (this.exit) instead of trusting the generator and the testing module to do this for us.

throw new Error('Generator should have failed.');
},
err => {
expect(err).to.match(/No models found in /);
Copy link
Member

Choose a reason for hiding this comment

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

As I understand expect().to.be.rejectedWith(/regexp/), it works exactly the same way as the new code which you wrote here using .then(onSuccess, onFailure). I personally prefer rejectedWith as it's less code to maintain and makes it more difficult to introduce an error.

What are the reasoning behind this change? Can we revert it please?

throw new Error('Generator should have failed.');
},
err => {
expect(err).to.match(/No repositories found in /);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, can we keep using rejectedWith instead?

true,
);
} catch (err) {
return this.exit(err);
Copy link
Member

Choose a reason for hiding this comment

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

Having to catch errors and report them via this.exit feels ugly to me, but if there isn't any better way, then I can live with it.

@bajtos
Copy link
Member

bajtos commented Jul 31, 2018

I'm just very peeved at yeoman that we have to essentially write our own error-handling system (this.exit) instead of trusting the generator and the testing module to do this for us.

@shimks could you please add a description of this problem to #844, the issue tracking all issues we have with Yeoman?

Sometimes it makes me wonder how much more difficult things have to get before we agree to get rid of Yeoman.

For context, the tests are failing because of the upgrades made between yeoman-generator 2 -> 3. The changes are listed here https://github.com/yeoman/generator/blob/master/lib/index.js#L433, where the error being thrown is emitted AND rejected. In v2, the error was emitted, and then a callback was called with the error. The problem is that in the test helper https://github.com/yeoman/yeoman-test/blob/master/lib/run-context.js#L121, the rejection is emitted AGAIN here.

I took a quick look. Isn't it possible to install error event handler on the generator instance?

Anyhow, these recent changes in yeoman look wrong to me, I think we should open an issue in yeoman, try to convince yeoman maintainers that the current behavior is not correct and get it fixed (possibly contributing the fix ourselves).

@bajtos bajtos changed the title Fix ci Fix CI: update yeoman-generator to 3.x, apply latest prettier formatting Jul 31, 2018
Copy link
Contributor

@shimks shimks left a comment

Choose a reason for hiding this comment

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

Not sure if my approval would count here, but your latest changes look good to me

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

👌

@bajtos
Copy link
Member

bajtos commented Jul 31, 2018

BTW: I changed the pull request title so that it better describe what is being changed. "Fix ci" is not very helpful when you see it as an entry in the list pull requests.

@virkt25 virkt25 merged commit 650b387 into master Jul 31, 2018
@virkt25 virkt25 deleted the fix-ci branch July 31, 2018 18:05
@virkt25
Copy link
Contributor Author

virkt25 commented Jul 31, 2018

BTW: I changed the pull request title so that it better describe what is being changed. "Fix ci" is not very helpful when you see it as an entry in the list pull requests.

Thank you for that! I think it just picked up the branch name instead of the commit message because there were multiple commits. I'll be more careful to update the PR Name in the future.

@shimks shimks mentioned this pull request Aug 1, 2018
@virkt25 virkt25 mentioned this pull request Aug 2, 2018
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

Successfully merging this pull request may close these issues.

None yet

5 participants