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

chore: migrate to github-actions #611

Merged
merged 87 commits into from
Apr 9, 2020
Merged

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Apr 3, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Migrate from azure-pipelines to github-actions

Breaking Changes

no

Additional Info

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #611 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #611   +/-   ##
=======================================
  Coverage   99.12%   99.12%           
=======================================
  Files           8        8           
  Lines         228      228           
  Branches       71       71           
=======================================
  Hits          226      226           
  Misses          2        2           

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 5aebf98...5aebf98. Read the comment docs.

@hiroppy

This comment has been minimized.

@hiroppy

This comment has been minimized.

@alexander-akait
Copy link
Member

That error is happening when tests are failed and watch not closed

@hiroppy

This comment has been minimized.

@hiroppy

This comment has been minimized.

@hiroppy hiroppy force-pushed the feature/modify-tests branch from c3816e6 to 69ebe09 Compare April 3, 2020 15:45
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Maybe we should move on github actions, just idea

@hiroppy
Copy link
Member Author

hiroppy commented Apr 3, 2020

Actually I want to move to github CI:)

@hiroppy
Copy link
Member Author

hiroppy commented Apr 3, 2020

Anyway, I'll resolve this problem and then I'll refactor many things.

@@ -370,7 +370,7 @@ describe('middleware', () => {
});
});

describe('should work with difference requests', () => {
describe.skip('should work with difference requests', () => {

This comment was marked as off-topic.

@hiroppy hiroppy force-pushed the feature/modify-tests branch 2 times, most recently from 7653c9f to c362302 Compare April 3, 2020 16:47
@hiroppy hiroppy changed the title Investigate tests test(middleware): use setImmediate for MacOS CI Apr 3, 2020
@hiroppy hiroppy force-pushed the feature/modify-tests branch from c362302 to cbb48a0 Compare April 3, 2020 16:53
@hiroppy

This comment has been minimized.

@hiroppy hiroppy changed the title test(middleware): use setImmediate for MacOS CI chore: migrate to github-actions Apr 5, 2020
@hiroppy
Copy link
Member Author

hiroppy commented Apr 5, 2020

Added CODECOV_TOKEN to settings/secret.

@hiroppy hiroppy force-pushed the feature/modify-tests branch from 44dc5d2 to 65bac2e Compare April 5, 2020 05:09
@hiroppy
Copy link
Member Author

hiroppy commented Apr 5, 2020

windows CIs seem flaky, need to investigate.

@hiroppy
Copy link
Member Author

hiroppy commented Apr 5, 2020

I'll refactor tests after merging this pr.

@hiroppy hiroppy force-pushed the feature/modify-tests branch 3 times, most recently from fc8136a to 1cd0835 Compare April 5, 2020 12:44
@hiroppy
Copy link
Member Author

hiroppy commented Apr 5, 2020

CI is green.

@hiroppy hiroppy marked this pull request as ready for review April 5, 2020 12:58
@hiroppy
Copy link
Member Author

hiroppy commented Apr 5, 2020

I'll modify github's setting of branch settings.

@hiroppy hiroppy requested a review from alexander-akait April 5, 2020 13:20
@hiroppy hiroppy mentioned this pull request Apr 6, 2020
6 tasks
uses: codecov/codecov-action@v1.0.6
with:
token: ${{ secrets.CODECOV_TOKEN }}
file: '**/junit.xml'
Copy link
Member

Choose a reason for hiding this comment

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

Let's union two files in one, they are small and can be combined

@alexander-akait
Copy link
Member

Seems segment fault appears after compiler = getCompiler(webpackMultiWarningConfig); in 3rd tests

@alexander-akait
Copy link
Member

webpack@5 doesn't uses chokidar with fsevents and no problems, so I think the problem in chokidar or fsevents

src/index.js Outdated
@@ -88,7 +88,8 @@ export default function wdm(compiler, options = {}) {
// eslint-disable-next-line no-param-reassign
callback = callback || noop;

context.watching.close(callback);
callback();
// context.watching.close(callback);
Copy link
Member

Choose a reason for hiding this comment

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

/cc @hiroppy The problem is here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good, so can we resolve this problem?

Copy link
Member

@alexander-akait alexander-akait Apr 8, 2020

Choose a reason for hiding this comment

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

I think bug in fsevents - fsevents/fsevents#314, need to fix it, probably will have to figure it out yourself, no problems with other os, today I will try to find a specific place of the problem

Copy link
Member

Choose a reason for hiding this comment

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

Maybe related - webpack/webpack#10037

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for investigation!

- next

jobs:
# lint:
Copy link
Member Author

Choose a reason for hiding this comment

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

Please uncomment lint job if possible

Copy link
Member

Choose a reason for hiding this comment

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

@hiroppy I will do it after we will fix the problem (before merge)

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, thanks

patch.js Outdated
};

`
);
Copy link
Member

Choose a reason for hiding this comment

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

@hiroppy yes, problem(s) in fsevents, I patched chokidar does not use fsevents and tests passed.

Choose a reason for hiding this comment

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

Now macos users would probably see webpack consuming 1GB of memory and tons of CPU.

Copy link
Member

Choose a reason for hiding this comment

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

@paulmillr Firstly, this is not true, I can take measurements again and demonstrate them to you. Secondly, I don’t see a desire to correct a bug or help us find it, so I don’t see any other choice how to turn off fsevents. I am always open and aimed at improvements and fix bugs, but for some reason you react negatively to my attempts to find a compromise.

Choose a reason for hiding this comment

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

That's because we've already fixed the bug in chokidar 3 — and you should update. There is no "unwilingness", there is just desire to not do the same work twice.

Go ahead and remove fsevents. You'll notice some bug reports in a couple weeks.

Copy link
Member

Choose a reason for hiding this comment

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

@paulmillr Why are you so negative to me, have I really done something bad to you? All I'm trying to do is find the error and fix it.

@hiroppy
Copy link
Member Author

hiroppy commented Apr 9, 2020

CI looks good

@alexander-akait
Copy link
Member

@hiroppy Let's merge 👍

@alexander-akait alexander-akait merged commit 43fa897 into master Apr 9, 2020
@alexander-akait alexander-akait deleted the feature/modify-tests branch April 9, 2020 13:57
@hiroppy
Copy link
Member Author

hiroppy commented Apr 9, 2020

@evilebottnawi Thank you for your investigation!

@hiroppy
Copy link
Member Author

hiroppy commented Apr 9, 2020

I'll copy to webapck-dev-server as well.

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