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

[WIP] Aegir@next #134

Merged
merged 39 commits into from
Aug 16, 2017
Merged

[WIP] Aegir@next #134

merged 39 commits into from
Aug 16, 2017

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented Jul 2, 2017

  • Extract eslint config into eslint-config-aegir
  • Improve cli interface
  • Refactor task runner to hopefully drop gulp
    • lint
    • build
    • test
    • coverage
    • release
    • docs
  • Upgrade uglify to uglify-es
  • Update to webpack@3
  • tests
    • setup
    • lint
    • build
    • test
    • coverage
    • release
    • docs
  • update docs
  • migration guide
  • test with js-ipfs
  • ensure all test options are available on release and coverage
  • remove sauce labs
  • remove old binary references

- Now uses eslint-config-aegir for rules.
- Updated to the latest version of standard
- Updated to the eslint@4
@dignifiedquire dignifiedquire added the status/in-progress In progress label Jul 2, 2017
@dignifiedquire
Copy link
Member Author

Progress is made:

screen shot 2017-07-02 at 23 57 46

screen shot 2017-07-03 at 00 09 16

@daviddias
Copy link
Member

wooooooot! :D ❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️
🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

@daviddias
Copy link
Member

Please make things like docs and pushing a changelog mandatory (i.e check for the Github token and fail if it is not there)

@daviddias
Copy link
Member

I like the new target flags 👍

@daviddias
Copy link
Member

daviddias commented Jul 5, 2017

  • Can aegir become one single command instead of multiple (aegir-lint, aegir-test, etc)?

@daviddias
Copy link
Member

@daviddias
Copy link
Member

@dignifiedquire
Copy link
Member Author

Can aegir become one single command instead of multiple (aegir-lint, aegir-test, etc)?

that's already the case with the new structure, it is now aegir lint instead of aegir-lint etc

@daviddias
Copy link
Member

I get excited every time I get a notification that there has been a new commit in this PR :D

@codecov
Copy link

codecov bot commented Jul 8, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@7b6419f). Click here to learn what that means.
The diff coverage is 33.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #134   +/-   ##
=========================================
  Coverage          ?   32.05%           
=========================================
  Files             ?       32           
  Lines             ?      418           
  Branches          ?        0           
=========================================
  Hits              ?      134           
  Misses            ?      284           
  Partials          ?        0
Impacted Files Coverage Δ
src/config/custom.js 100% <ø> (ø)
src/release/push.js 0% <ø> (ø)
src/release/docs.js 0% <ø> (ø)
src/release/index.js 0% <0%> (ø)
src/release/github.js 0% <0%> (ø)
src/release/publish.js 0% <0%> (ø)
src/docs/build.js 0% <0%> (ø)
src/config/karma.conf.js 0% <0%> (ø)
src/coverage/providers/codecov.js 0% <0%> (ø)
src/release/contributors.js 0% <0%> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b6419f...d4b8c31. Read the comment docs.

@daviddias daviddias requested a review from kumavis July 9, 2017 10:15
@@ -1,5 +1,5 @@
'use strict'

module.exports = {
timeout: 80 * 1000
timeout: 40 * 1000
Copy link
Member

Choose a reason for hiding this comment

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

What about 5 secs and any test that needs more either should get it running below 5 secs or explicitly signal that? (and I say 5 and not 2 just because of CI vs user machines)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have seen many many tests that take longer than 5 seconds on both CI and locally so I am not sure this will just result in tons of test manually extending the timeout, which is worse imho than setting a relative high value here

Copy link
Member

Choose a reason for hiding this comment

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

I think that is a symptom of us not having something to remind us that certain operations should be fast and/or that tests should be a unit and not a massive set of operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

reduced to 5 seconds and we get a red flag on node tests for slow tests:

screen shot 2017-07-09 at 17 38 16

@dignifiedquire dignifiedquire changed the title [WIP] Next level of ageir [WIP] Aegir@next Jul 10, 2017
}
```

You can also specify `hooks.browser` and `hooks.node` if you have a different setup for browser and/or node based tests.
Copy link
Member

Choose a reason for hiding this comment

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

what about hooks.electron and hooks.cordova? Will it be easy to extend to support those runtimes as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we support running tests in them yes

README.md Outdated

### Renamed binaries

`aegir` is now a single binary with subcommands. While something like `aegir-test` still works, we recommend to switch to the new `aegir test` syntax.
Copy link
Member

Choose a reason for hiding this comment

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

Since this is going to be a major release, you might just want to clean all of that old code anyway :)

README.md Outdated

### Switch from Mocha to Jest

Because the mocha binary was the source of many pains tests in node are now run using [Jest](https://facebook.github.io/jest/). All important features should work as before, but if you were relying on a mocha specific feature it might break now.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@dignifiedquire
Copy link
Member Author

Status update, most of the work inside this project is done. But before we can move forward we need to fix jestjs/jest#2059 which impacts us in a strong way as most of our tests are written in this particular style that causes issues in Jest. I have started looking into this, but not figured out all details yet.

@dignifiedquire
Copy link
Member Author

Jest work is happening here: https://github.com/dignifiedquire/jest/tree/fix/async-tests

@daviddias
Copy link
Member

One thing to consider is that we keep bumping the limit of nested callbacks to 8, maybe set that as the sane default?

@daviddias
Copy link
Member

When you say that docs is complete, does it include a markdown exported version to API.md?

@daviddias
Copy link
Member

Checking in here. I know this PR is blocked due to jest issues. Would it be good to bring mocha again so that we can have all of the other nice things in this PR?

@dignifiedquire
Copy link
Member Author

Would it be good to bring mocha again so that we can have all of the other nice things in this PR?

We would loose some of the nice things and one of the main issues I moved away from mocha is that it had this great issue where it couldn't be properly killed, so that issue would be partially back. Going to work on the jest pr in the next days and see if I can either get it merged or otherwise publish my fork in a way we can start using it

@dignifiedquire
Copy link
Member Author

Merging this, this branch was open long enough

@dignifiedquire dignifiedquire merged commit 6266215 into master Aug 16, 2017
@dignifiedquire dignifiedquire deleted the next branch August 16, 2017 11:40
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.

2 participants