[FEATURE ember-improved-instrumentation] Add link-to instrumentation

[asakusuma]

Adding basic instrumenting to link-to.

[rwjblue]

Restarted sauce labs job.

[asakusuma]

For some reason this test is failing in Microsoft Edge. Not sure why, the test does not seem to use link-to...

[asakusuma]

Here is the error:
RangeError: Array length must be a finite positive integer

Video: https://saucelabs.com/jobs/7266308c4dc94740acc91265e645c45f

I'm going to just manually step through the test to try to figure out why this is happening. May also try to get a hold of a windows machine.

Any history with Microsoft Edge causing problems for ember test runs?

[asakusuma]

@rwjblue I'm trying to figure out what actual lines of code map to each entry in the ember.debug.js stack trace in the saucelab video. However, the stack has changed between saucelab runs. I would assume this means that either the failures are not deterministic or that saucelab rebases before every run.

Neither of the stack traces line up with what I'm seeing in my ember.debug.js locallly. I have not rebased since pushing to the PR branch. Any tips on debugging a saucelab output?

[asakusuma]

Some weird stuff is going on in MS Edge. I tried running the tests on a windows machine with Edge and could not reproduce the issue. Eventually I figured out that that backburner.run was somehow getting passed no arguments, causing this line to throw the RangeError: Array length must be a finite positive integer. The source of the errors is run.join(target, action, ...args).

When I change the syntax for run.join(target, action, ...args) to something that should be equivalent, the tests pass. Feels like a browser bug to me. I don't see anyway in which run.join(target, action, ...args) can result in an array of length zero.

[mixonic]

Should not be a runloop here. The runloop is inside the click handler.

[mixonic]

Also a few places above.

[mixonic]

is the value ever anything but a function? Can this simply check truthiness?

Making before optional would call for a documentation update as well.

[homu]

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

[krisselden]

Asa, can you rebase?

[asakusuma]

Did some more digging today and I believe this is a bug in MS Edge. Looks like saucelabs is on an unsupported version of MS Edge. I've tweeted at them to see if they've got some plans to upgrade.

[asakusuma]

@rwjblue any insight on how we can get Saucelabs to upgrade their version of Microsoft Edge? Version 13 is pretty out of date and no longer supported.

https://en.wikipedia.org/wiki/Microsoft_Edge#Release_history

Edit: I tweeted at them on Friday https://twitter.com/asakusuma/status/726207486768750593

[rwjblue]

They are using 13.10586 of EdgeHTML, which correlates to version 25 in that chart.

[asakusuma]

It says in the saucelabs metadata that 13.10586 is the "browser version." How do we know that the version is referring to EdgeHTML and not the generic browser version?

[rwjblue]

I suppose we don't know for 100% certainty, but the version number matching exactly what is used by Edge 25 (and the only time that number exists on the wiki page you referenced) would be a heck of a coincidence.

In addition, a forum comment by Sauce Labs support staff mentioned that Edge 25 was available as of March of this year: https://support.saucelabs.com/customer/portal/questions/16186272-edge-25-availability.

[asakusuma]

You're right @rwjblue https://twitter.com/saucelabs/status/727232878380208128

My windows machine is on Edge 20, I'll upgrade and see if I can reproduce this.

[homu]

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

[asakusuma]

@stefanpenner I've described the complete issue in this comment, but an even simpler description would be:

Function foo gets called via apply with the following arguments array: [a, b].concat(args), where a and b are any variables, and args is an array of any length. Inside foo, arguments.length is 0. This should not be possible since the arguments array is hardcoded to be at least length 2.

EDIT: There is another function bar, that is called first, which directly passes through to foo. Here is the actual passthrough function that is getting called.

[asakusuma]

And to connect my general description to the actual issue at hand, foo in my example is run loop join in this issue.

[stefanpenner]

@asakusuma so the arity changes on edge?

[stefanpenner]

cc @jdalton / @bterlson seems like we may be running into an issue with IE Edge. Specifically outlined here any thoughts/ideas?

[asakusuma]

@stefanpenner it would appear so. Not sure if the actual arity changes or if the representation of arity changes. In other words, arguments.length is zero, but I'm not sure if arguments[0] would actually have the intended value.

Issue is not reproducible when using a debugger.

[asakusuma]

@stefanpenner as I suspected, if the guard is moved, it doesn't work. See the commit pass/fail status.

[homu]

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

[homu]

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

[stefanpenner]

@stefanpenner as I suspected, if the guard is moved, it doesn't work. See the commit pass/fail status.

thanks for trying

[asakusuma]

@stefanpenner @nathanhammond @krisselden looks like the closure action test migration has "fixed" the issue. Tests are passing now without the guard hack.

[homu]

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

[jdalton]

cc @jdalton / @bterlson seems like we may be running into an issue with IE Edge. Specifically outlined here any thoughts/ideas?

This sounds familiar. It would help if you all have a small repro.
Have you tried on insider builds of Windows 10?

[asakusuma]

I've had no luck re-creating in a small repro. Unfortunately, the issue has only ever surfaced when running the entire test suite, not just the single test.

I have not tried on an insider build. I'll see if I can get IT to arrange that. I've created a branch that can reproduce the issue.

Are there any current known issues that might inform a method for reproducing this issue?

[homu]

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

[homu]

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

[asakusuma]

@krisselden @stefanpenner what needs to happen to get this merged?

[stefanpenner]

@asakusuma it looks good to me, but im not terribly familiar with this. @krisselden should likely sign-off.

[chadhietala]

@asakusuma I think it should be fine now that the tests will actually run when the feature flag is on. 👍