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

[Pls send halp!] Cleaning up the test output #16391

Closed
12 of 17 tasks
chancancode opened this issue Mar 22, 2018 · 24 comments
Closed
12 of 17 tasks

[Pls send halp!] Cleaning up the test output #16391

chancancode opened this issue Mar 22, 2018 · 24 comments

Comments

@chancancode
Copy link
Member

chancancode commented Mar 22, 2018

♻️ ♻️ ♻️ Please consider the environment before printing this Github issue. ♻️ ♻️ ♻️

screenshot_3_22_18__1_46_pm

Currently, the test output is very noisy – even for green builds. This makes it very difficult find any actual failures, especially on small screens and on phones.

Let's try to clean it up!

Here are the useless output I have found based on this test run. Leave a comment about what you are looking into (🔒) so others won't duplicate the effort.

A lot of these probably have the same root cause (for example, because of how the default Ember.onError works) and probably require some refactoring in the test infrastructure.

  • Build phase
    • 'container' is imported by tmp/rollup-cache_path-I6PjJI6s.tmp/run_loop.js, but could not be resolved – treating it as an external dependency
    • 'ember-debug' is imported by tmp/rollup-cache_path-l0Z4sygU.tmp/index.js, but could not be resolved – treating it as an external dependency
    • 'assert' is imported from external module '@glimmer/util' but never used
    • 'assert' and 'SERIALIZATION_FIRST_NODE_STRING' are imported from external module '@glimmer/util' but never used (🔒 @James-Byrne)
  • Test phase
    • WARNING: Library "magic" is already registered with Ember.
    • console.trace
    • DEPRECATION: Should not throw [deprecation id: test] (🔒 @Draggha )
    • (node:6689) Warning: Possible EventEmitter memory leak detected. * listeners added. Use emitter.setMaxListeners() to increase limit (🔒 @lukecoy )
    • (node:6689) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error: Navigation Timeout Exceeded: 900ms exceeded
    • Error: Element .does-not-exist not found. (🔒 @KamiKillertO )
    • Error: Catch me (🔒 @KamiKillertO )
    • Error: the error (🔒 @KamiKillertO )
    • Testing paused. Use resumeTest() to continue.
    • The library version banner on app boot (🔒 @Draggha )
      DEBUG: -------------------------------
      DEBUG: Ember  : *
      DEBUG: jQuery : *
      DEBUG: -------------------------------
      
    • Error while processing route: * (🔒 @allthesignals )
    • More context objects were passed than there are dynamic segments for the route: stink-bomb
      `
    • WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities; please ensure that values being bound are properly escaped. For more information, including how to disable this warning, see https://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes. Style affected: * (🔒 @KamiKillertO )
@James-Byrne
Copy link

James-Byrne commented Mar 22, 2018

I can have a look at

Build Phase

'assert' and 'SERIALIZATION_FIRST_NODE_STRING' are imported from external module '@glimmer/util' but never used

@Draggha
Copy link
Contributor

Draggha commented Mar 22, 2018

I'll take a stab at

Test Phase

Testing paused. Use resumeTest() to continue.

Draggha added a commit to Draggha/ember.js that referenced this issue Mar 22, 2018
Draggha added a commit to Draggha/ember.js that referenced this issue Mar 22, 2018
@Draggha
Copy link
Contributor

Draggha commented Mar 22, 2018

I'd like to take a stab at

Test Phase

WARNING: Library "magic" is already registered with Ember.

The console.trace message beneath it should be resolved with the same fix if I saw correctly.

Draggha added a commit to Draggha/ember.js that referenced this issue Mar 22, 2018
@KamiKillertO
Copy link
Contributor

I Can have a look at Error: Element .does-not-exist not found.

@acorncom
Copy link
Contributor

Thanks folks, I’ve updated the issue accordingly

@KamiKillertO
Copy link
Contributor

I'll also take Error: Catch me and Error: the error. They are related to Error: Element .does-not-exist not found.

Draggha added a commit to Draggha/ember.js that referenced this issue Mar 23, 2018
solves the task "WARNING: Library "magic" is already registered with Ember."
Draggha added a commit to Draggha/ember.js that referenced this issue Mar 23, 2018
solves the task "Testing paused. Use resumeTest() to continue."
@Draggha
Copy link
Contributor

Draggha commented Mar 23, 2018

I think I have an idea how to handle the library version banner on app boot:

DEBUG: -------------------------------
DEBUG: Ember  : *
DEBUG: jQuery : *
DEBUG: -------------------------------

Draggha added a commit to Draggha/ember.js that referenced this issue Mar 23, 2018
solves the task: `the library version banner on app boot`
@Draggha
Copy link
Contributor

Draggha commented Mar 23, 2018

I can have a look at:

DEPRECATION: Should not throw [deprecation id: test]

:)

Draggha added a commit to Draggha/ember.js that referenced this issue Mar 23, 2018
solves the task: `DEPRECATION: Should not throw [deprecation id: test]`
@acorncom
Copy link
Contributor

Thanks @Draggha, noted 👍

@KamiKillertO
Copy link
Contributor

I take

WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities; ...

rwjblue added a commit that referenced this issue Mar 24, 2018
rwjblue added a commit that referenced this issue Mar 24, 2018
[CLEANUP beta] #16391 Cleaning up the test output
rwjblue added a commit that referenced this issue Mar 24, 2018
[CLEANUP beta] #16391 Cleaning up the test output
rwjblue added a commit that referenced this issue Mar 24, 2018
[CLEANUP beta] #16391 Cleaning up the test output
rwjblue added a commit that referenced this issue Mar 26, 2018
[CLEANUP beta]  #16391 Cleaning up the test output
@allthesignals
Copy link
Contributor

allthesignals commented Mar 30, 2018

I'll take a stab at Error while processing route: *

Edit: No dice, not sure how to suppress the error message. Since it's throwing an error on an instance of another object (a Route), it may not be getting caught by the error callback... open to suggestions!

this.add(
  'route:c',
  Route.extend({
    afterModel() {
      throw new Error('transition failure');
    }
  })
);

Edit2: Got a PR for an attempt but want to make sure it's ok approach before I apply everywhere.

@acorncom
Copy link
Contributor

Thanks @allthesignals, noted

@acorncom
Copy link
Contributor

Ok folks, we've got 8 items left here. Any takers? @James-Byrne looks like there isn't a PR yet for your task, did we miss linking the PR over? Do you still want to take the one with your name on it or should we unlock that and let someone else take a stab?

@KamiKillertO
Copy link
Contributor

I can take :

node:6689) UnhandledPromiseRejectionWarning: and More context objects were passed than there are dynamic segments for the route: stink-bomb 😄

@allthesignals
Copy link
Contributor

allthesignals commented Mar 31, 2018

@acorncom this test actually checks the window console for error messages: https://github.com/emberjs/ember.js/blob/master/packages/ember/tests/routing/decoupled_basic_test.js#L3471. Should it be rewritten to check these messages differently? Or skipped? Not sure how conservative I should try to be. Thx.

Edit: Ah, @KamiKillertO, I think this is overlap of what you've taken on.

@lukecoy
Copy link
Contributor

lukecoy commented Apr 3, 2018

I wouldn't mind taking a look at this one:

Test Phase
(node:6689) Warning: Possible EventEmitter memory leak detected. * listeners added. Use emitter.setMaxListeners() to increase limit

@rwjblue
Copy link
Member

rwjblue commented Apr 3, 2018

@lukecoy - you got it!

@KamiKillertO
Copy link
Contributor

It's seems that (node:6689) UnhandledPromiseRejectionWarning: Unhandled promise rejection is already fixed. I cannot find this error in the logs

rwjblue added a commit that referenced this issue Apr 5, 2018
[CLEANUP beta] #16391 Cleaning up the test output
lukecoy pushed a commit to lukecoy/ember.js that referenced this issue Apr 6, 2018
lukecoy added a commit to lukecoy/ember.js that referenced this issue Apr 6, 2018
@KamiKillertO
Copy link
Contributor

I will have a look to 'container' is imported by tmp/rollup-cache_path-I6PjJI6s.tmp/run_loop.js, but could not be resolved – treating it as an external dependency but i'm not sure where to look 😞

lukecoy added a commit to lukecoy/ember.js that referenced this issue Apr 6, 2018
lukecoy added a commit to lukecoy/ember.js that referenced this issue Apr 9, 2018
lukecoy added a commit to lukecoy/ember.js that referenced this issue Apr 9, 2018
lukecoy added a commit to lukecoy/ember.js that referenced this issue Apr 9, 2018
lukecoy added a commit to lukecoy/ember.js that referenced this issue Apr 9, 2018
rwjblue added a commit that referenced this issue Apr 9, 2018
[CLEANUP beta] #16391 Cleaning up the test output
@acorncom
Copy link
Contributor

Hey folks, we're getting close here. Anyone blocked on things and need help in a given area?

@KamiKillertO
Copy link
Contributor

KamiKillertO commented May 10, 2018

🙋I'm stuck with 'container' is imported by tmp/rollup-cache_path-I6PjJI6s.tmp/run_loop.js, but could not be resolved – treating it as an external dependency

[edit] I've just check on masterand this message no longer appear 😄

@acorncom
Copy link
Contributor

@rwjblue @chancancode any guidance here? I've got even less context on the snag than @KamiKillertO does 😉

@pixelhandler
Copy link
Contributor

@Draggha @James-Byrne @KamiKillertO @acorncom @allthesignals @chancancode @lukecoy @rwjblue is this still an issue, perhaps we should close, what do you think?

@chancancode Is there anything that remains TODO?

@chancancode
Copy link
Member Author

Looks like a lot of these are addressed, I am okay closing the tracking issue and filing new ones as needed. I opened #17016 and #17017. Looks like a few of these still have outstanding PRs waiting for reviews also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants