Skip to content

[WIP] E2E tests for init and migrate using docker and soren#169

Closed
evenstensberg wants to merge 3 commits into
masterfrom
stdin-tests
Closed

[WIP] E2E tests for init and migrate using docker and soren#169
evenstensberg wants to merge 3 commits into
masterfrom
stdin-tests

Conversation

@evenstensberg

Copy link
Copy Markdown
Member

No description provided.

@okonet okonet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd use e2e lowercased everywhere especially for file names but otherwise it looks great to me!

@evenstensberg

Copy link
Copy Markdown
Member Author

Gotcha, so what's left is to add this for migrate and change to lower case.

@DanielaValero

Copy link
Copy Markdown
Contributor

Maybe also fixing the lintin issues reported by codacy/pr?
-> https://www.codacy.com/app/okonet/webpack-cli/pullRequest?prid=748619

Comment thread CONTRIBUTING.md
Make sure to have installed docker before you continue.

```sh
$ docker build -t webpack-cli .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The installation kept on failing on my environment due to the protocol of the dependencies installed via git.

Once I added the +https it worked:

"yeoman-generator": "git+https://github.com/ev1stensberg/generator.git#Feature-getArgument"

and

"recast": "git+https://github.com/kalcifer/recast.git#bug/allowbreak",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, changing those once we're post-yeoman migration to webpack-addons

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, that's expected

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd still recommend to add the +https, so the docker testing works until the migration is complete, we never know for sure when it will happen, so better have a working docker testing until then, for all the users, instead of having people to add the +https on their own and not commit the changes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Could you post a screenshot of your terminal? Should work regardless

@evenstensberg

Copy link
Copy Markdown
Member Author

Some errors with migrate, there's still errors, index.test in /transforms are invalid. The https issue should be done in a sep PR

@DanielaValero

Copy link
Copy Markdown
Contributor

Hi @ev1stensberg

sure: @https issue of the package.json

about the broken tests: I'd not recommend to merge into master the PR with the broken tests, as it would add up to the "broken window" syndrome, and would also imply that we are ok with having broken tests in the master code, which is not really advisable.

@evenstensberg

Copy link
Copy Markdown
Member Author

I dunno what the broken window syndrome is :/

@DanielaValero

Copy link
Copy Markdown
Contributor

Hi @ev1stensberg

Just noticed that the broken tests come from master. I will get a PR to fix them.

The broken window syndrome recommends that broken tests, poor quality, lintin issues, and such similarities, should be fixed right away. Leaving them would progressively add up, and add up, until the point that are many broken windows to fix and is more expensive to fix them all in one go, as would have been to fix a single broken window when it was found.

The codacy is complaining about some lintin errors, which perhaps makes sense to fix in my pr #173

Apart from this, is your PR complete?

@evenstensberg

Copy link
Copy Markdown
Member Author

In the meeting, we decided to get input from @okonet on how his experience with Ink were, so I'd like a update on that first!

@evenstensberg
evenstensberg deleted the stdin-tests branch October 5, 2017 21:03
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.

3 participants