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.

Sign up for GitHub

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

Silences warning logs and adds additional earlier skip check if no detected BCD updates #909

Merged
merged 8 commits into from
Dec 14, 2023
Merged

Silences warning logs and adds additional earlier skip check if no detected BCD updates #909

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bc8b26d
set log message default to quiet
ChrisC Nov 29, 2023
e4944b9
adds helper to detect contradictions
ChrisC Nov 29, 2023
ace7e41
adds check for "preview" statement
ChrisC Nov 29, 2023
846fb2e
catches specific version_added updates over generic support statements
ChrisC Dec 4, 2023
09eb95a
walks back edits to "normal" update unit test
ChrisC Dec 4, 2023
388dc62
stops using "contradictions" terminology
ChrisC Dec 7, 2023
1a15d19
fixes logic for handling "preview" statements
ChrisC Dec 7, 2023
f8a0f4a
Merge remote-tracking branch 'upstream/main' into only-log-warnings-o…
ChrisC Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions scripts/update-bcd.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
findEntry,
getSupportMap,
getSupportMatrix,
hasSupportMatrixContradictions,
inferSupportStatements,
splitRange,
update,
Expand Down Expand Up @@ -821,6 +822,138 @@ describe("BCD updater", () => {
});
});

describe("hasSupportMatrixContradictions", () => {
it("detects contradictions with nonexistent support statements", () => {
assert.isTrue(
hasSupportMatrixContradictions(new Map([["80", true]]), {
version_added: null,
}),
);

assert.isTrue(
hasSupportMatrixContradictions(new Map([["80", true]]), undefined),
);
});

it("skips null support claims", () => {
assert.isFalse(
hasSupportMatrixContradictions(new Map([["80", null]]), {
version_added: "≤80",
}),
);

assert.isFalse(
hasSupportMatrixContradictions(
new Map([
["79", false],
["80", true],
["81", true],
["82", null],
]),
{
version_added: "≤80",
},
),
"skips new null test result",
);
});

it("detects contradictions in statements with boolean values", () => {
assert.isFalse(
hasSupportMatrixContradictions(new Map([["80", false]]), {
version_added: false,
}),
"skips generic false statements",
);

assert.isTrue(
hasSupportMatrixContradictions(new Map([["80", true]]), {
version_added: true,
}),
"catches specific support updates over generic true statements",
);

assert.isTrue(
hasSupportMatrixContradictions(
new Map([
["80", false],
["81", true],
]),
{version_added: false},
),
);
});

it("detects contradictions in statements with string values", () => {
assert.isTrue(
hasSupportMatrixContradictions(
new Map([
["79", false],
["80", false],
["81", true],
]),
{
version_added: "≤80",
},
),
);

assert.isFalse(
hasSupportMatrixContradictions(
new Map([
["79", false],
["80", false],
["81", true],
]),
{
version_added: "81",
},
),
);

assert.isFalse(
hasSupportMatrixContradictions(
new Map([
["79", false],
["80", true],
["81", true],
]),
{
version_added: "≤80",
},
),
);

assert.isTrue(
hasSupportMatrixContradictions(
new Map([
["79", false],
["80", true],
["81", true],
["82", false],
]),
{
version_added: "≤80",
},
),
"detects possible support removal",
);

assert.isTrue(
hasSupportMatrixContradictions(
new Map([
["79", false],
["80", true],
["81", true],
]),
{
version_added: "preview",
},
),
);
});
});

describe("update", () => {
const supportMatrix = getSupportMatrix(reports, bcd.browsers, overrides);
let bcdCopy;
Expand Down
83 changes: 77 additions & 6 deletions scripts/update-bcd.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,12 @@ const reason = (
message: ReasonMessageFactory,
args: Omit<Reason, "message"> = {},
): ReasonFactory => {
return (value) => ({message: message(value), skip: true, ...args});
return (value) => ({
message: message(value),
skip: true,
quiet: true,
...args,
});
};

const isReasonFactory = (
Expand Down Expand Up @@ -679,6 +684,48 @@ const skipCurrentBeforeSupport = skip("currentBeforeSupport", ({
}
});

export const hasSupportMatrixContradictions = (
ChrisC marked this conversation as resolved.
Show resolved Hide resolved
versionMap: BrowserSupportMap,
simpleStatement?: SimpleSupportStatement,
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be the signature if we pass in defaultStatements:

Suggested change
simpleStatement?: SimpleSupportStatement,
statements: SimpleSupportStatement[],

It could be an empty array, but not null. An example of what an array might be:

"support": {
  "chrome": [
    {
      "version_added": "30"
    },
    {
      "version_added": "10",
      "version_removed": "20"
    }
  ]
}

If we may have tests result showing support in Chrome 110, and the lack of support in Chrome 29, that is consistent with BCD. But if we have test results showing support in Chrome 9, that is a contradiction.

In my following comments, I'll assume that we iterate over versionMap, skip any nulls, and inside that loop iterate over statements, which is usually zero or one item, but possibly more.

Copy link
Member

Choose a reason for hiding this comment

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

@ChrisC do you plan to address this? If so I'll hold off with review of the recent changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChrisC do you plan to address this? If so I'll hold off with review of the recent changes.

@foolip I did try to address this and was hoping that with some smart sorting of the statements and versions, this would be simple to implement. Unfortunately, that approach did not work reliably.

Instead, I'm going to add another provide... method after we get the defaultStatements to infer supported and unsupported ranges to check each version against. That will be the most robust solution for checking version support against multiple default statements.

I would prefer to do that in a separate PR if possible (since I think it will be quite a bit more code to add), but if you have a strong preference for keeping it in this PR, I can continue with that work here.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds great @ChrisC, and fine to do it in a separate PR since it's not as simple as I thought initially.

Copy link
Member

Choose a reason for hiding this comment

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

Here's untested code of how I thought testing against an array of statements (defaultStatements) would probably work:

// Test a single version (string) against a BCD statements array.
// Note that order of statements does not matter.
function isSupported(version, statements) {
  for (const { version_added, version_removed } of statements) {
    if (version_added && compare(version, version_added, '>=') {
      if (version_removed && compare(version, version_removed, '>=') {
        continue;
      }
      return true;
    }
  }
  return false;
}

function hasSupportUpdates(versionMap, statements) {
  for (const [version, hasSupport] of versionMap.entries()) {
    if (hasSupport === null) {
      continue;
    }
    if (hasSupport !=== isSupported(version, statements) {
      return true;
    }
  }
}

In other words, for each true or false test result for a specific version (from collector results), test it against the support statements, once range at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more or less what I tried. However, take the array of statements you provided as an example ☝🏽.

"support": {
  "chrome": [
    {
      "version_added": "30"
    },
    {
      "version_added": "10",
      "version_removed": "20"
    }
  ]
}

Let's say you're testing version 10 with a a test result of hasSupport === true against this set of support statements. This should not show any contradictions. However, when tested for support against each statement individually, it would return false when tested against the the statement of version_added: 30 and then send out a false positive signal for the need to update the statements. (Note that it would return true against the other statement.)

I suppose you could collect all the comparisons when iterating through the statements, and assume that as long as there's no inconsistency between the individual statement comparisons, there there's no need to update them. But that seemed a bit brittle and non-specific to me. See another approach where we test against ranges more specifically in #967.

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look at your new PR, but for now just want to say that I don't think we should test a version ("10" in your example) against a part of a support statement, only the full array as one test.

If there is a contradiction we do need to figure out how which part of a support statement to update, or if we should add/remove a part, but I consider that a problem of handling more cases correctly, not identifying when an update is needed and logging.

) => {
if (!simpleStatement || simpleStatement.version_added === null) {
return true;
}

if (simpleStatement.version_added === "preview") {
foolip marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

const contradictions: string[] = [];
for (const [version, supportAssertion] of versionMap.entries()) {
if (supportAssertion === null) {
continue;
}

if (typeof simpleStatement.version_added === "boolean") {
if (!simpleStatement.version_added && !supportAssertion) {
continue;
} else {
contradictions.push(version);
}
}

if (typeof simpleStatement.version_added === "string") {
const simpleAdded = simpleStatement.version_added.replace("≤", "");
if (compareVersions(version, simpleAdded, "<") && supportAssertion) {
contradictions.push(version);
}
if (
compareVersions(version, simpleAdded, ">=") &&
supportAssertion === false
) {
contradictions.push(version);
}
}
}
return contradictions.length > 0;
};

const persistInferredRange = provideStatements(
"inferredRange",
({
Expand Down Expand Up @@ -919,6 +966,19 @@ export const update = (
}
}),
skipBrowserMismatch(options.browser),
provideAllStatements,
provideDefaultStatements,
skip("supportMatrixMatchesDefaultStatements", ({
shared: {versionMap},
defaultStatements: [simpleStatement],
}) => {
if (!hasSupportMatrixContradictions(versionMap, simpleStatement)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pass in defaultStatements here, so that we can detect contradictions when there are multiple version ranges involved. We aren't able to update all such cases yet, but that should then result in a warning.

return reason(
({path, browser}) =>
`$${path} skipped for ${browser} because support matrix matches current BCD support data`,
);
}
}),
provide("inferredStatements", ({shared: {versionMap}}) =>
inferSupportStatements(versionMap),
),
Expand All @@ -931,8 +991,6 @@ export const update = (
}
}),
skipReleaseMismatch(options.release),
provideAllStatements,
provideDefaultStatements,
skip("zeroDefaultStatements", ({
inferredStatements: [inferredStatement],
defaultStatements,
Expand Down Expand Up @@ -971,13 +1029,26 @@ export const update = (
persistAddedOver,
persistRemoved,
clearNonExact(options.exactOnly),
skip("noStatement", ({statements}) => {
skip("noStatement", ({
statements,
defaultStatements: [simpleStatement],
shared: {versionMap},
}) => {
if (!statements?.length) {
if (hasSupportMatrixContradictions(versionMap, simpleStatement)) {
return reason(
({browser, path}) =>
`${path} skipped for ${browser} with unresolved differences between support matrix and BCD data. Possible intervention required.`,
{
quiet: false,
},
);
}
return reason(
({browser, path}) =>
`${path} skipped for ${browser}: no reason identified`,
`${path} skipped for ${browser}: no known reason identified. Possible intervention required.`,
{
quiet: true,
quiet: false,
},
);
}
Expand Down