Correct support for mirroring

[jugglinmike]

Hi there! I'm interested in contributing to BCD with authentic test results for the Meta Quest browser, so I've been working with the "mirror" feature (as introduced to BCD here and integrated to this project in gh-2039). I filed gh-2261 to report my initial experience, but even following an expeditious fix by @queengooborg, I suspect there may still be some bugs.

To facilitate this discussion, I've written nine unit tests for the mirroring behavior implemented by the update-bcd script. The first commit in this pull request introduces those tests, including "expected" values dictated by the current behavior of the application. However, as mentioned above, some of the current behavior seems incorrect to me. The second commit modifies the "expected" values according to how I believe the feature was conceived to behave. This commit does not modify the application logic (I'd like to verify my interpretation with the maintainers before attempting to implement a solution), so the new tests fail with the second commit applied.

(I'm also happy to iterate on the organization of the test code, ideally in parallel to the discussion about the expected behavior of the application code. I've tried to strike a balance between concision and legibility--a never-ending struggle in test design. Also: although the existing tests each tend to validate a bunch of behaviors, I tried to express the conditions here with a series of focused cases. That said, I wasn't able to completely avoid dependencies on the surrounding state because browser data, although parameterized in this project, is hard-coded into the BCD project.)

[foolip]

Thanks Mike, and nice to see you here! I haven't looked at the current behavior after @queengooborg made some changes, but I've commented on what I think should be the behavior.

Note that the ambiguity when having just a single browser version's results applies outside of mirroring too.

[foolip]

Without results from Chrome Android 85, I don't think this is a valid assumption. The updater script should have concluded "support in Chrome Android 86, earlier support unknown" and not see a contradiction in need of fixing by updating the data.

The updater code isn't really structured in a way that makes this easy to fix however, see #571

[jugglinmike]

Without results from Chrome Android 85, I don't think this is a valid assumption. The updater script should have concluded "support in Chrome Android 86, earlier support unknown" and not see a contradiction in need of fixing by updating the data.

I think I understand. The expected value for this test should be "mirror", right?

The updater code isn't really structured in a way that makes this easy to fix however, see #571

The code currently produces the value "mirror" for this case (as per the first commit on this branch), so if that matches the expectation, then there's nothing to fix here.

[foolip]

Yes, the way I'd like the updater to work is that when we have incomplete information that doesn't contradict the existing data, it is left alone. (An early goal of being able to derive everything in BCD from test results has gone out the window by now, but might explain some choices in the design.)

What you might want to do is to tweak these tests to actually work with a false-to-true transition, to test the behavior I think you were originally going for here.

The case of incomplete information can be tested much more lightly, maybe just one "no change made" test, and perhaps one for the case where we can say the current data is wrong, but not exactly what is right. Such tests need not involve mirroring though.

[jugglinmike]

The full title for this test includes "supported upstream (without flags)" and "supported in downstream test results", so it was not my intention to test a false-to-true transition here.

On the other hand, the seventh test title includes "unsupported upstream" and "supported in downstream test results." That does describe a false-to-true transition.

The term "transition" may be a bit ambiguous for the "mirror" feature since the value under test is informed by two other values: the "upstream" browser support (Chrome 85 in this case) and the support for the prior release of the "downstream" browser (Chrome Android 85). In all nine of the tests I've written so far, the support for the prior release is omitted, and I believe that's interpreted as "unknown."

I'm wondering if you are looking for tests where the transition takes place solely on the basis of "downstream" browser data. More concretely, where the Chrome Android browser expresses negative support directly (not via mirroring), and the test results describe positive support. Or in code:

const actual = mirroringCase({
  support: {
    chrome_android: false
  },
  downstreamResult: true
});
assert.deepEqual(
  actual,
  bcdFromSupport({
    chrome_android: {version_added: 86}
  })
);

That'd be a false-to-true transition that occurred within a browser and without any consideration for the upstream results. If so, it seems beyond the scope of a patch for mirroring--do you agree?

[foolip]

How to deal with incomplete information is indeed beyond the scope of fixing mirroring, but the test expectations you've written seem to suggest you'd like to change it, unless I'm wrong about current behavior.

The way I'd suggest approaching mirroring is:

• Mirror the data from upstream
• Do everything the same way as if mirroring didn't exist
• Optionally at the end, detect if the new data is the same as mirroring would produce and if so replace with "mirror". (This isn't super important because one can always run the mirror migration script in BCD later to fix it.)

My comments are about how the "do everything" bit should work, since the tests poke at that. The "transition" I'm talking about is not changing a value in BCD, but the logic in inferSupportStatements. If we have results for Chrome Android 85 and 86 and observe a false→true transition in test results between the two, then we can infer {version_added: "86"}. But if we have results from only 86, then the correct inference is {version_added: "≤86"}. That won't pass the BCD lint, but it does force a human to look into the uncertainty.

If you generate results for both 85 and 86 you can deal only with the simpler case, which I suspect is the more important one to get fixed.

[jugglinmike]

My goal is to contribute data about the Meta Quest browser (formerly Oculus Quest). I'm focused on the mirroring behavior of update-bcd script because it's my understanding that (due to this patch), I'll be relying heavily on that specific functionality. As I understand it, the incomplete information in this case would be compatibility data for prior releases of the Meta Quest browser.

That's why all of these tests describe conditions where the "downstream" support value is set to "mirror" and why none include prior support values downstream. I thought I was covering all the conditions for my use case exhaustively, but I'm getting the sense that either my understanding of the use case is inaccurate or that the underlying problem is larger than what can be observed (or addressed) by my use case alone.

Does that match your understanding?

[foolip]

I think I might be getting you entangled in some details that I enjoy but that aren't super relevant to you, like what exactly to do with partial test results.

Perhaps the concrete bug you're seeing would help guide us to the right things to test. I guess you're running update-bcd with some results from Meta Quest, and it's making the wrong change in some cases? Can we start with one case to get warmed up?

[jugglinmike]

I can't collect test results from that browser at the moment; my participation in this repository has so far been proactive. I've been trying to enumerate all of the situations that the script might encounter when it receives the first set of results from the Quest browser. I may have identified some conditions which are more exotic than we'll see with authentic data, but I'm fairly confident that many represent mainline use cases. Any problems with those will block progress when I do have results in hand.

An example would be when a test result for a "downstream" browser is negative, and the "upstream" support is positive. The script ought to change "mirror" to {version_added: false}, but it currently does nothing. (That case is described by the second row of the table in gh-2261 and by the test named "BCD updater, update, mirror, supported upstream (without flags), unsupported in downstream test results" in this patch.)

It may be that we're closer than we think. Between implementing support for the new "flags" data policy, learning that existing support data should be ignored, and generally understanding the internals a bit better, I've been able to write a patch that corrects the problems. I'll be able to share it here once the "flags" patch is merged. In the mean time, there's one more expected value I'd like to revisit.

[foolip]

Yes, we should change "mirror" to {version_added: false} if applying the mirroring results in enabled-by-default support, which is contradicted by test results. This is the framework I'd suggest in general, to adjust data to be consistent with the evidence, and otherwise leave the data alone, even if we're not able to verify it.

[jugglinmike]

Thanks, Philip, nice to be working with you again! It's been too long since I've had the pleasure of a foolip review :)

[jugglinmike]

Without results from Chrome Android 85, I don't think this is a valid assumption. The updater script should have concluded "support in Chrome Android 86, earlier support unknown" and not see a contradiction in need of fixing by updating the data.

I think I understand. The expected value for this test should be "mirror", right?

The updater code isn't really structured in a way that makes this easy to fix however, see #571

The code currently produces the value "mirror" for this case (as per the first commit on this branch), so if that matches the expectation, then there's nothing to fix here.

[jugglinmike]

Also here: a "support statement" is either "mirror" or a "simple support statement", and a "simple support statement" must be an Object value. So this expected value ought to be expressed with chrome_android: {version_added: false}.

[foolip]

@jugglinmike do you want to fix the tests and un-draft this now?

[jugglinmike]

@foolip I've corrected the tests and included a fix for the bug. Merging gh-2317 will almost certainly cause conflicts with this patch, and I'll be available to resolve those (they ought to be trivial, anyway).

[foolip]

Looks good in terms of the changes. Have you compared what the differences are in the results produced before/after this change? And do you want #2317 or this landed first?

[foolip]

If you like, feel free to define const clone = (value) => JSON.parse(JSON.stringify(value)) like in #2317 and use it here/elsewhere.

[foolip]

@jugglinmike #2317 is merged now. There are no conflicts, but do you want to add a clone() and show some concrete differences that result from the changes?

[jugglinmike]

Sure! clone is now applied consistently throughout. As for the data, here's a summary of the changes

I started with this data from the Quest browser. Using that, here's what the "update" script on the main branch does to BCD:

$ git log -n2 --oneline add-quest-from-unpatched-collector
cf127aea3 (add-quest-from-unpatched-collector) Add Quest data using unpatched BCD collector
48a4a8d0a (upstream/main, upstream/HEAD, main) Remove "default values" notes for Web Audio constructors (#17811)
$ git show --shortstat add-quest-from-unpatched-collector
commit cf127aea34094bc821726d3a8b84af6b223f1c30 (add-quest-from-unpatched-collector)
Author: Mike Pennisi <mike@mikepennisi.com>
Date:   Tue Sep 20 13:06:58 2022 -0400

    Add Quest data using unpatched BCD collector

 9 files changed, 44 insertions(+), 15 deletions(-)

And here's what the patched "update" script does to BCD:

$ git log -n2 --oneline add-quest-from-patched-collector
713cd89ca (HEAD -> add-quest-from-patched-collector) Add Quest data using patched BCD collector
48a4a8d0a (upstream/main, upstream/HEAD, main) Remove "default values" notes for Web Audio constructors (#17811)
$ git show --shortstat add-quest-from-patched-collector
commit 713cd89ca705bce7b365eb66abb467e454ae782c (HEAD -> add-quest-from-patched-collector)
Author: Mike Pennisi <mike@mikepennisi.com>
Date:   Tue Sep 20 13:09:57 2022 -0400

    Add Quest data using patched BCD collector
    
    See https://github.com/foolip/mdn-bcd-collector/pull/2280

 111 files changed, 1231 insertions(+), 404 deletions(-)

And here's a direct comparison of the two:

$ git diff --shortstat add-quest-from-unpatched-collector add-quest-from-patched-collector
 103 files changed, 1187 insertions(+), 389 deletions(-)

There's too much to include inline, so I've uploaded the full diff as a GitHub Gist.

[jugglinmike]

The required check for this job was cancelled. I don't know why that happened, and the output in CI doesn't help much:

Checking formatting...
All matched files use Prettier code style!
Error: The operation was canceled.

[foolip]

@jugglinmike in GitHub actions, when one job fails, the others are canceled. And it looks like the macOS job failed due to a timeout in npm ci. I'll just re-run the jobs, see if that was a network outage.

[foolip]

Thanks @jugglinmike for the diff of the changes before/after these changes. Those looks sensible, but a number of them are probably due to problems with the tests like #2347. But I have pretty high confidence in the changes based on that diff and the unit tests, of course.

[jugglinmike]

Thank you, @foolip! I'll be scrutinizing the results over the coming days, and I'll keep you posted about any other anomalies I find.

[foolip]

@jugglinmike thanks, looking forward to more bug reports! 😆