Skip to content

add initial ember-cli-addon-tests #173

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

Closed
wants to merge 2 commits into from

Conversation

csantero
Copy link
Contributor

The one test this adds:

  • boots an app called ts1 using ember-cli-typescript, with a pre-existing typescript file at app/add.ts
  • downloads the final app javascript file
  • ensures that the js file contains the expected transpiled output for a module called ts1/add

@csantero
Copy link
Contributor Author

Looks like the 5 minute timeout was too short for Appveyor. :(

.then(response => {
expect(response.statusCode).to.equal(200);
let parsed = esprima.parseScript(response.body);
let bodyStatements = parsed.body
Copy link
Member

@dfreeman dfreeman Mar 16, 2018

Choose a reason for hiding this comment

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

I wonder if we could get away with something like this to make it less scary to maintain/add more tests like this in the future?

let parsed = esprima.parseScript(response.body);
let expected = esprima.parseScript(`
  export function add(a, b) {
    return a + b;
  }
`);

expect(parsed).to.deep.equal(expected);

EDIT: Ah right, we're dealing with post-Babel output as well. Might still be worth a try with the corresponding define(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfreeman Good idea! I'll add that in.

@csantero
Copy link
Contributor Author

@dfreeman I refactored the test as per your suggestion. How does that look?

Also, Appveyor seems to be complaining that yarn is missing. I'm guessing yarn is available globally for travis but not for appveyor?

@dfreeman
Copy link
Member

@csantero Looks good! Re: yarn on Appveyor, I've had success by adding an install entry for it in the past (...apparently 😄)

@chriskrycho
Copy link
Member

Hey, sorry this sat for so long unattended, @csantero. AppVeyor does have yarn available in its test environment, so that shouldn't be the issue. Let's get this updated with latest from master and push it forward this week?

@csantero
Copy link
Contributor Author

@chriskrycho I don't know what's causing the failure. It works locally

@chriskrycho
Copy link
Member

I'll see if I can poke at it next week if no one beats me to it.

Quick request for future reference: please add commits rather than rebasing for PRs to this repo. It's much easier to track what comments were in reference to that way! (No harm done, just figured I'd note it. Should put a note that way in a CONTRIBUTING.md, I think.)

@csantero
Copy link
Contributor Author

@chriskrycho interesting. I hate it when people update PRs against my repos with merge commits that bring in lots of commits from master. I guess it's personal preference?

@chriskrycho
Copy link
Member

Ha! Yes. I prefer that because it keeps the context. GitHub (and, for that matter, git diff and so on) is usually smart enough not to show changes pulled in that way.

@csantero
Copy link
Contributor Author

Also, my current theory about these failures is that they're just too slow. I don't think this PR is going to be viable until ember-cli-addon-tests sees some major performance improvements.

@dfreeman
Copy link
Member

We landed the moral equivalent of this work in #202, so I'm going to go ahead and close out this PR. Thanks for forging the path here, @csantero!

@dfreeman dfreeman closed this Jul 25, 2018
@mike-north
Copy link
Contributor

@dfreeman Are we sure that we have an addon acceptance test, not just an app one? I don't see it in the commits for #202

@dfreeman
Copy link
Member

We don't currently have an addon acceptance test, but this PR didn't introduce one either :)

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.

4 participants