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

Modernize install-blueprint-task.js test #8557

Merged

Conversation

RichardOtvos
Copy link
Contributor

No description provided.

@RichardOtvos
Copy link
Contributor Author

@Turbo87 Do we want to replace the the .to.eventually.equal type of assertions with an await and to.equal type ones as part of this task?

Like this:

it('resolves "foobar" by looking up the "foobar" blueprint locally', function() {
      let foobarBlueprint = { name: 'foobar blueprint' };
      td.when(task._lookupBlueprint('foobar')).thenResolve(foobarBlueprint);
      return expect(task._resolveBlueprint('foobar')).to.eventually.equal(foobarBlueprint);
});

with something like this

it('resolves "foobar" by looking up the "foobar" blueprint locally', async function() {
      let foobarBlueprint = { name: 'foobar blueprint' };
      td.when(task._lookupBlueprint('foobar')).thenResolve(foobarBlueprint);
      
      const resolvedBlueprint = await task._resolveBlueprint('foobar');
      expect(resolvedBlueprint).to.equal(foobarBlueprint);
});

@Turbo87
Copy link
Member

Turbo87 commented Apr 6, 2019

yes, I think something like this makes sense:

it('resolves "foobar" by looking up the "foobar" blueprint locally', async function() {
      let foobarBlueprint = { name: 'foobar blueprint' };
      td.when(task._lookupBlueprint('foobar')).thenResolve(foobarBlueprint);
      expect(await task._resolveBlueprint('foobar')).to.equal(foobarBlueprint);
});

@RichardOtvos RichardOtvos force-pushed the modernize-install-blueprint-task-test branch from 9252fe1 to dfb1a45 Compare April 6, 2019 14:16
@RichardOtvos RichardOtvos mentioned this pull request Apr 7, 2019
@RichardOtvos
Copy link
Contributor Author

@Turbo87 Do you see any benefits in changing the way we handle testing promise rejections, from

      // normal function (and what we currently have in the codebase)
      return expect(task.ensureBower()).to.be.rejectedWith('foobar');

to

      //async function
      await expect(task.ensureBower()).to.be.rejectedWith('foobar');

The benefit I see in the second way, is that it would mean you always use async functions when you test Promises (rather than only use them when testing for resolved promises), but I'm not sure if it's worth switching for that alone.

I've experimented with some other ways to test rejections, but everything I could think of was a lot more verbose or invasive than chai-as-promised .rejectedWith assertion.
I'm open to ideas if you can think of anything else. :)

@Turbo87
Copy link
Member

Turbo87 commented Apr 7, 2019

@RichardOtvos yep, that second snippet looks 👌 to me! :)

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

👏 nice work, just a few small comments

tests/unit/tasks/install-blueprint-test.js Outdated Show resolved Hide resolved
tests/unit/tasks/install-blueprint-test.js Outdated Show resolved Hide resolved
@RichardOtvos
Copy link
Contributor Author

This should be ready, let me know if you require any more changes.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

muchas gracias 👏

@Turbo87 Turbo87 merged commit 4d3df0f into ember-cli:next Apr 12, 2019
@Turbo87 Turbo87 changed the title #8511 Modernize install-blueprint-task.js test Modernize install-blueprint-task.js test Apr 12, 2019
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.

None yet

2 participants