Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore(app): update several components #2756

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

Merged
Awk34 merged 1 commit into angular-fullstack:master from zeripath:issue-2691-simple
Jul 27, 2018

Conversation

Copy link
Contributor

@zeripath zeripath commented Jul 24, 2018
edited
Loading

Part of #2691

Copy link
Contributor Author

Hmm... it looks like this one is crashing the continuous integration. Not sure quite why...

Copy link
Member

Awk34 commented Jul 24, 2018

Copy link
Member

@Awk34 Awk34 left a comment

Choose a reason for hiding this comment

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

Take the package-lock.json changes out. I prefer to only make those changes for releases.

@zeripath zeripath force-pushed the issue-2691-simple branch 2 times, most recently from daba839 to 6dca3c5 Compare July 24, 2018 20:45
Copy link
Contributor Author

Agh the mocha changes mean this is going to be slightly less simple. I guess I'll take another look tomorrow

Copy link
Member

Awk34 commented Jul 24, 2018

Yeah, just need to make sure the servers shut down after all the tests are complete.

I really appreciate all of the contributions!

Copy link
Contributor Author

No thank you for the generator. I'm yak shaving before I actually use the generator properly.

Copy link
Member

Awk34 commented Jul 25, 2018 via email

Haha 'yak shaving', that's a new one
...
On Tue, Jul 24, 2018, 14:11 zeripath ***@***.***> wrote: No thank you for the generator. I'm yak shaving before I actually use the generator properly. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2756 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFSeAnZF27F4R-Dl7uomsETLyCPjoEAYks5uJ42EgaJpZM4VdPN5> .

Copy link
Contributor Author

zeripath commented Jul 26, 2018
edited by Awk34
Loading

@Awk34 So I'm still having difficulty figuring out what's causing this problem. Looking at templates/app/mocha.global.js the tests should close down express after every set but it seems that somehow they're not.

I've tried sticking in delays etc. I can't even see which test is causing a restart of the express server...

This is quite frustrating...

Copy link
Member

Awk34 commented Jul 26, 2018

@zeripath my assumption about what's breaking this could be wrong. Perhaps we should test with Mocha 3.5.3 to make sure it still passes, then with 4.0.0 to make sure it starts failing.

Copy link
Member

Awk34 commented Jul 26, 2018

If it is breaking because of what I think, then the proper investigation path would be to attach some debugger to be able to see what's keeping the process alive after the tests are done

Copy link
Contributor Author

Ah I think I've found it.

In src/test/main.test.js:151: (amongst others)

before(function() {
 return runGen(null, {
 copyConfigFile: true,
 options: {
 skipInstall: true,
 skipConfig: true
 }
 }).then(_dir => {
 dir = _dir;
 lintResult = runCmd('npm run lint');
 clientTestResult = runCmd('npm run test:client');
 serverTestResult = runCmd('npm run test:server');
 });
 });

This causes a race somewhere in creating the express server. If these are moved out into their respective it(...) then the race stops happening and the crash goes away.

I haven't yet checked whether doing this changes the results of the serverTestResult... (i.e. is it still run in the correct directory.)

Copy link
Member

Awk34 commented Jul 26, 2018

Hmm okay, I can see that causing issues. I did this to start the test commands so that they'd run in parallel and hopefully save time. Hopefully separating the server test command won't add too much time to the test runs.

Copy link
Contributor Author

Cool. It looks like I've done it!

(The package-lock.json change that creeped in by accident has been removed too. )

Copy link
Contributor Author

I think this patch might also fix issue #2746

Awk34 reacted with thumbs up emoji

@Awk34 Awk34 merged commit cc718b9 into angular-fullstack:master Jul 27, 2018
@zeripath zeripath deleted the issue-2691-simple branch July 27, 2018 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Awk34 Awk34 Awk34 approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /