Skip to content

Test on Windows through AppVeyor, and use NPM prepublishOnly rather than prepublish#139

Merged
tbroyer merged 2 commits intomasterfrom
ci-tweaks
Jul 10, 2018
Merged

Test on Windows through AppVeyor, and use NPM prepublishOnly rather than prepublish#139
tbroyer merged 2 commits intomasterfrom
ci-tweaks

Conversation

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jul 7, 2016

Actually, I'm not really sure about this, because AppVeyor has a really strange concept of "account" vs "user".
The checks currently are at https://ci.appveyor.com/project/tbroyer/itowns2/build/1.0.1 (notice that it's linked to my account, not to some sort of organization). I can rename my "account" to "iTowns", and according to their documentations I (the "user")could be a "member" of another, personal, "account"; but I'm really not sure they'd let me create that other account.

So, well, this PR adds AppVeyor configuration, and that's what's important.
But ultimately maybe someone who only intends to use AppVeyor for iTowns should sign-up there, rename his account to iTowns and create the job.
In the mean time, I'm OK with having itowns2 under my personal AppVeyor account (as I intend to eventually use AppVeyor for other projects, I'm not going to rename my account), so if everyone's OK with seeing my name in the AppVeyor URLs, then that's OK.

@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 12, 2016

@iTowns/reviewers Should we move this forward? or should I delete the hooks that currently build each push and PR? (and fails early by lack of appveyor.yml configuration)

@gmaillet
Copy link
Collaborator

I'm not sure that AppVeyor is necessary for now if it's only for the building process (with no real tests).

@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 13, 2016

FWIW:

That said, I'm OK with delaying adding AppVeyor until we do have tests!

@gchoqueux gchoqueux added the wip 🚧 Still being worked on label Nov 10, 2016
@autra
Copy link
Contributor

autra commented Jul 9, 2018

@tbroyer do you mind if I close this? It has been open for 2 years, and I'm not sure it's really relevant any more (we currently use pupeeter for browser tests). WDYT?

@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 9, 2018

Not being a Windows users, I don't have a strong opinion on this.

I however still think it could be useful to have automated feedback from Windows:

  • that developers on Windows still have a working environment
  • that tests pass on Windows

How about first trying to update that PR?

@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 9, 2018

Fwiw, updated PR ran OK on AppVeyor: https://ci.appveyor.com/project/tbroyer/itowns2/build/1.0.595

@tbroyer tbroyer changed the title Test on Windows through AppVeyor, and update Travis to Node 6 Test on Windows through AppVeyor, and use NPM prepublishOnly rather than prepublish Jul 9, 2018
Copy link
Contributor

@autra autra left a comment

Choose a reason for hiding this comment

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

Fwiw, updated PR ran OK on AppVeyor: https://ci.appveyor.com/project/tbroyer/itowns2/build/1.0.595

Oh so we might be nearer merging that than I initially thought, great!

In the mean time, I'm OK with having itowns2 under my personal AppVeyor account (as I intend to eventually use AppVeyor for other projects, I'm not going to rename my account), so if everyone's OK with seeing my name in the AppVeyor URLs, then that's OK.

I don't mind. What's the worst that could happen? That you close your account? In this case, we'll just switch I guess. Or maybe AppVeyor did introduce something like organization account in the meantime?

Where is the url configured btw?

"transpile": "cross-env BABEL_DISABLE_CACHE=1 babel src --out-dir lib",
"start": "cross-env NODE_ENV=development webpack-dev-server -d --inline --hot",
"prepublish": "npm run build && npm run transpile"
"prepublishOnly": "npm run build && npm run transpile"
Copy link
Contributor

Choose a reason for hiding this comment

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

From the commit message:

Note that in CI (Travis and AppVeyor), "npm test" will
run "npm run build", so this scenario is still "tested".

I wholeheartedly support this change, although if I'm not mistaken, the transpile part is not run. If we switch to prepublishOnly, we should either add transpile to build or to test, or maybe both btw (is there a script name for that?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't transpiling already done when compiling? I mean, Babel runs as part of the Webpack build; it does not output ES5 to disk, but this is mostly equivalent in all other aspects, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is, you're right. Actually, I thought we needed the lib/ folder for unit tests, but actually sources are compiled on the fly by mocha apparently. I think I mixed up with the old version of integration tests, before we used puppeeter.

@autra
Copy link
Contributor

autra commented Jul 9, 2018

also, @tbroyer apparently something is wrong in the AppVeyor repo, because github webhook receives {"message":"PR and project repositories do not match."}. Maybe it's missing the renaming itowns -> itowns2 ?

@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 9, 2018

I don't mind. What's the worst that could happen? That you close your account? In this case, we'll just switch I guess. Or maybe AppVeyor did introduce something like organization account in the meantime?

They did! https://www.appveyor.com/docs/team-setup/
I added @iTowns/psc as "admin" and @iTowns/reviewers as "user".
Builds will now be at https://ci.appveyor.com/project/iTowns/itowns (only change I made from the default configuration: I checked "ignore branches without appveyor.yml")

I also removed the webhook and deleted the project from my personal account.

@zarov
Copy link
Contributor

zarov commented Jul 9, 2018

It seems to block some PR, why is that ? See for example #798

Copy link
Contributor

@autra autra left a comment

Choose a reason for hiding this comment

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

LGTM! Glad to see this happening.

@tbroyer can you edit your commit messages by prefixing them with test: for the first one and chore: for the second? We're trying to adopt a convention for them to automatically generate changelogs, see #518.

Thanks again for resurrecting this!

tbroyer added 2 commits July 10, 2018 10:18
Only test on latest stable Node release, contrary to Travis
which runs latest stable and latest LTS.
Now that Node LTS has support for that new event, we can
make the switch and avoid building and transpiling when
running "npm install" locally when updating dependencies.

Note that in CI (Travis and AppVeyor), "npm test" will
run "npm run build", so this scenario is still "tested".
@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 10, 2018

can you edit your commit messages

Done

@tbroyer tbroyer merged commit 075dd6a into master Jul 10, 2018
@tbroyer tbroyer deleted the ci-tweaks branch July 10, 2018 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wip 🚧 Still being worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants