CLI: fixup shutdown/interrupt code #7019

stefanpenner · 2017-05-02T23:14:01Z

fix tests
add regression test

… failing on master This is a temporary change to unblock Ember Data See ember-cli/ember-cli#7019 for a more long term fix

…er-cli and is failing on master This is a temporary change to unblock Ember Data See ember-cli/ember-cli#7019 for a more long term fix

Previously the command's run promise would not resolve, and subsequent then handlers responsible for producing instrumentation would never run. Cleanup occurred due to capture-exit. Now our exit handler not only cleans up, but also finishes the run promise chain.

stefanpenner · 2017-05-03T01:59:06Z

r? @hjdivad / @rwjblue / @ro0gr

chadhietala · 2017-05-03T07:22:01Z

tests/unit/cli/cli-test.js

@@ -228,6 +235,31 @@ describe('Unit: CLI', function() {
    });
  });

+  describe.only('command crash', function() {


Remove the only

Ooo. Will fix

hjdivad

LGTM.

My understanding of how interrupt handlers were executed was wrong.

stefanpenner · 2017-05-03T16:17:03Z

Windows CI seems unhappy, re-running. If not I can take a look tonight when i have a windows machine handy.

ro0gr · 2017-05-03T22:13:35Z

tests/unit/cli/cli-test.js

+      };
+
+      return ember(['custom']).then(function() {
+        expect(false, 'should have rejected').to.eql(false);


Shouldn't one of these false be a true? Seems like this line will always pass if a promise isn't rejected.

actually we should use https://github.com/domenic/chai-as-promised for tests like that. e.g.

return expect(ember(['custom'])).to.be.rejected;

stefanpenner · 2017-05-04T20:10:30Z

tests/unit/cli/cli-test.js

+      };
+
+      return ember(['custom']).then(function() {
+        expect(false, 'should have rejected').to.eql(true);


switch to chai-as-promised.

…er-cli and is failing on master This is a temporary change to unblock Ember Data See ember-cli/ember-cli#7019 for a more long term fix (cherry picked from commit ef68161)

ro0gr · 2017-05-15T23:37:49Z

I've tested this pr against several known issues:

ember fastboot does not terminate with ember-cli >= 2.13 ember-fastboot/ember-cli-fastboot#386
[BUGFIX beta] Skip test which doesn't play nicely with the latest em… emberjs/data#4965

The good news is that these issues are addressed.

The bad news is that it breaks testem integration on windows.

I've noticed it in "ember new foo, test, SIGINT exits with error and clears tmp/" test failure. The test on itself passes but afterEach hook fails with the following message:

√ ember new foo, test, SIGINT exits with error and clears tmp/ (145694ms)

"after each" hook for "ember new foo, test, SIGINT exits with error and clears tmp/"

1 passing (4m)
1 failing

Acceptance: smoke-test "after each" hook for "ember new foo, test, SIGINT exits with error and clears tmp/":
AssertionError: expected "C:\Users\kudgo\workspace\ember-cli\tmp\some_cool_app_clone-KeX18ufj.tmp" to not exist

Literally afterEach is unable to delete temporary application made for test. The reason for this is that testem doesn't kill phantomjs process anymore. This process locks test application directory.

testem.log - before this pr

info Test'em starting..
info Seeking for config file...
info Starting ci
info Starting server
info Waiting for tests.
info Start triggered test run.
info Running tests...
info Tests running.
info spawning: phantomjs - [ 'C:\Users\kudgo\workspace\ember-cli\node_modules\testem\assets\phantom.js',
'http://localhost:7357/5383/tests/index.html?hidepassed' ]
info New client connected: PhantomJS 2.1 5383
info tryAttach PhantomJS 2.1 5383
info received SIGINT signal
info Closing browser PhantomJS.
WARN PhantomJS closed 1
info Process PhantomJS terminated. 1 null

testem.log - this pr

info Test'em starting..
info Seeking for config file...
info Starting ci
info Starting server
info Waiting for tests.
info Start triggered test run.
info Running tests...
info Tests running.
info spawning: phantomjs - [ 'C:\Users\kudgo\workspace\ember-cli\node_modules\testem\assets\phantom.js',
'http://localhost:7357/5096/tests/index.html?hidepassed' ]
info New client connected: PhantomJS 2.1 5096
info tryAttach PhantomJS 2.1 5096
info

In the second log we miss received SIGINT signal entry which causes the problem.

I'm in process of investigation. Any ideas would be awesome.

ro0gr · 2017-05-15T23:40:21Z

It's not just a test setup issue. It also reproducible by manual verification

homu · 2017-05-17T13:26:18Z

☔ The latest upstream changes (presumably #7048) made this pull request unmergeable. Please resolve the merge conflicts.

ro0gr · 2017-05-22T21:57:59Z

The bad news is that it breaks testem integration on windows.

After some testing it turned out that it breaks only with phantomjs. Chrome launcher is disposed properly.
Seems like some kind of timing issue. Wrapping of interruption handler in timeout "fixes" the problem:

        removeInterruptionHandler = onProcessInterrupt.addHandler(() => {
          return new RSVP.Promise(function(resolve) {
            setTimeout(() => {
              command.onInterrupt();
              resolve();
            }, 1000);
          });

But it seems like a bad solution.

I've checked how does "ctrl+c" work against ember-cli@2.12 the problem persist there. The reason why this issue didn't appear earlier in tests is that we didn't have smoke test for ember test interruption.

This pr looks good for me but if we merge most likely it will break windows CI.

ro0gr · 2017-05-22T22:01:20Z

lib/cli/cli.js

-    })
-    .catch(this.logError.bind(this));
+    }).finally(() => {
+      onProcessInterrupt.teardown();


What is the reason for additional teardown here? https://github.com/ember-cli/ember-cli/pull/7019/files#diff-16a41bfbe414387d773df099f941217eR162 should do all the necessary cleanup?

ro0gr · 2017-06-29T20:48:39Z

@stefanpenner I believe now with headless chrome this should pass under appveyor

kellyselden · 2017-07-13T20:26:05Z

@ro0gr Rebased on master.

ro0gr · 2017-07-13T22:14:50Z

@kellyselden great!

There is one failing test which breaks here .

I think it should be re-written to match updated will-interrupt-process api.

Before:

      // ...
      removewillInterruptProcess.addHandler(cb);
      td.verify(process.stdin.setRawMode(true));

      willInterruptProcess.removeHandler(cb);
      td.verify(process.stdin.setRawMode(false));

      td.verify(process.stdin.setRawMode(), {
        ignoreExtraArgs: true,
        times: 2,
      });

After:

      // ...
      let removeHandler = willInterruptProcess.addHandler(cb); // `addHandler` now yields `removeHandler`
      td.verify(process.stdin.setRawMode(true));

      removeHandler(cb);
      td.verify(process.stdin.setRawMode(false));

      td.verify(process.stdin.setRawMode(), {
        ignoreExtraArgs: true,
        times: 2,
      });

I believe this should work.

Saying the truth I find this test fragile. We could test it much more better if merge #7086 first.

stefanpenner · 2017-07-13T23:48:48Z

@ro0gr its merged!

Turbo87 · 2018-05-17T06:52:22Z

closing due to inactivity and conflicts

stefanpenner force-pushed the interrupt-fun branch 5 times, most recently from 1153c20 to 2ca57a8 Compare May 2, 2017 23:47

bmac added a commit to bmac/data that referenced this pull request May 3, 2017

Skip test which doesn't play nicely with the latest ember-cli and is…

d8e5003

… failing on master This is a temporary change to unblock Ember Data See ember-cli/ember-cli#7019 for a more long term fix

bmac mentioned this pull request May 3, 2017

[BUGFIX beta] Skip test which doesn't play nicely with the latest em… emberjs/data#4965

Merged

stefanpenner force-pushed the interrupt-fun branch 4 times, most recently from 939e45d to 20087cb Compare May 3, 2017 01:56

stefanpenner mentioned this pull request May 3, 2017

Command interruption #6615

Merged

11 tasks

stefanpenner requested review from hjdivad, rwjblue and Turbo87 May 3, 2017 02:00

chadhietala reviewed May 3, 2017

View reviewed changes

hjdivad approved these changes May 3, 2017

View reviewed changes

ro0gr reviewed May 3, 2017

View reviewed changes

stefanpenner force-pushed the interrupt-fun branch 4 times, most recently from 93f754f to 7f008fd Compare May 4, 2017 04:10

stefanpenner commented May 4, 2017

View reviewed changes

stefanpenner force-pushed the interrupt-fun branch from 7f008fd to 03d2681 Compare May 4, 2017 22:02

ro0gr mentioned this pull request May 13, 2017

ember fastboot does not terminate with ember-cli >= 2.13 ember-fastboot/ember-cli-fastboot#386

Closed

ro0gr reviewed May 22, 2017

View reviewed changes

ro0gr mentioned this pull request May 28, 2017

Reject on interruption #7069

Closed

ro0gr mentioned this pull request Jun 7, 2017

Mock process #7086

Merged

7 tasks

hjdivad mentioned this pull request Jul 11, 2017

ember-cli notes 13 july 2017 emberjs/core-notes#172

Merged

This was referenced Jul 13, 2017

update ember-cli et al mainmatter/ember-simple-auth#1395

Merged

Enable blueprint tests again once ember-cli bug is fixed mainmatter/ember-simple-auth#1396

Closed

[BUGFIX release] fix shutdown/interrupt code

7274291

kellyselden force-pushed the interrupt-fun branch from 03d2681 to 7274291 Compare July 13, 2017 20:23

ro0gr mentioned this pull request Jul 14, 2017

[BUGFIX release] fix shutdown/interrupt code (rebased) #7229

Closed

2 tasks

ro0gr mentioned this pull request Aug 6, 2017

improve removal of interruption handler #7285

Closed

Turbo87 closed this May 17, 2018

rwjblue deleted the interrupt-fun branch December 19, 2019 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: fixup shutdown/interrupt code #7019

CLI: fixup shutdown/interrupt code #7019

stefanpenner commented May 2, 2017 •

edited

Loading

stefanpenner commented May 3, 2017

chadhietala May 3, 2017

stefanpenner May 3, 2017

hjdivad left a comment

stefanpenner commented May 3, 2017

ro0gr May 3, 2017

stefanpenner May 3, 2017

Turbo87 May 3, 2017

stefanpenner May 4, 2017

ro0gr commented May 15, 2017 •

edited

Loading

ro0gr commented May 15, 2017

homu commented May 17, 2017

ro0gr commented May 22, 2017

ro0gr May 22, 2017

ro0gr commented Jun 29, 2017

kellyselden commented Jul 13, 2017

ro0gr commented Jul 13, 2017

stefanpenner commented Jul 13, 2017

Turbo87 commented May 17, 2018

CLI: fixup shutdown/interrupt code #7019

CLI: fixup shutdown/interrupt code #7019

Conversation

stefanpenner commented May 2, 2017 • edited Loading

stefanpenner commented May 3, 2017

chadhietala May 3, 2017

Choose a reason for hiding this comment

stefanpenner May 3, 2017

Choose a reason for hiding this comment

hjdivad left a comment

Choose a reason for hiding this comment

stefanpenner commented May 3, 2017

ro0gr May 3, 2017

Choose a reason for hiding this comment

stefanpenner May 3, 2017

Choose a reason for hiding this comment

Turbo87 May 3, 2017

Choose a reason for hiding this comment

stefanpenner May 4, 2017

Choose a reason for hiding this comment

ro0gr commented May 15, 2017 • edited Loading

ro0gr commented May 15, 2017

homu commented May 17, 2017

ro0gr commented May 22, 2017

ro0gr May 22, 2017

Choose a reason for hiding this comment

ro0gr commented Jun 29, 2017

kellyselden commented Jul 13, 2017

ro0gr commented Jul 13, 2017

stefanpenner commented Jul 13, 2017

Turbo87 commented May 17, 2018

stefanpenner commented May 2, 2017 •

edited

Loading

ro0gr commented May 15, 2017 •

edited

Loading