Page MenuHomePhabricator

Failing CI: schema compatibility test in analytics/legacy/test
Closed, ResolvedPublic

Description

This error is causing unrelated CI verification to fail:

1) Schema Compatibility in Repository ./jsonschema/                                                                                                                        
     analytics/legacy/test                                                                                                                                                 
       Major Version 1                                                                                                                                                     
         1.0.0 must be compatible with 1.2.0:                                                                                                                              
                                                                                                                                                                           
    AssertionError [ERR_ASSERTION]: Removed a required property at path: .required                                                                                         
    + expected - actual                                                                                                                                                    
                                                                                                                                                                           
     [                                                                                                                                                                     
       "$schema"                                                                                                                                                           
    +  "event"                                                                                                                                                             
       "meta"                                                                                                                                                              
     ]                                                                                                                                                                     
                                                                                                                                                                           
    at new AssertionError (internal/assert.js:269:11)                                                                                                                      
    at isRequiredCompatible (node_modules/@wikimedia/jsonschema-tools/lib/tests/compatibility.js:22:15)                               
    at isCompatible (node_modules/@wikimedia/jsonschema-tools/lib/tests/compatibility.js:47:21)
    at Context.it (node_modules/@wikimedia/jsonschema-tools/lib/tests/compatibility.js:87:37)

Indeed, the 1.0.0 schema was missing the required item event. I can't tell why this suddenly appears, since no recent changes have been made in this directory, in the tests, or jsonschema-tools.

Git bisect tells me this commit is responsible for the failure,
778a881737a1f9c845795cb41bc00200ed81b989 Add MobileWebUIActionsTracking to analytics/legacy T267347 , but that patch only concerns the jsonschema/analytics/legacy/mobilewebuiactionstracking directory.

Event Timeline

What is strange is that 1.0.0 does not need to be compatible with 1.2.0, that is wrong. I think something is incorrectly sorting the schema versions on the CI servers.

@awight you can repro this locally? I cannot!

I get:

analytics/legacy/test
  Major Version 1
    ✓ 1.1.0 must be compatible with 1.0.0
    ✓ 1.2.0 must be compatible with 1.1.0

@awight you can repro this locally? I cannot!

I can, and it starts very clearly at the patch linked above, using git bisect and npm install && npm run-script test.

npm 6.4.11
node v10.21.0
debian 10.7

Let me know if I can help, try patches, dump diagnostics, etc.

I get:

analytics/legacy/test
  Major Version 1
    ✓ 1.1.0 must be compatible with 1.0.0
    ✓ 1.2.0 must be compatible with 1.1.0

I see what you're saying, yes that's the problem then. Failed runs have,

analytics/legacy/test
  Major Version 1
    1) 1.0.0 must be compatible with 1.2.0
    ✓ 1.1.0 must be compatible with 1.0.0

For revisions which pass, I get the correct comparisons:

analytics/legacy/test                                           
  Major Version 1                                                                                                                                                       
    ✓ 1.1.0 must be compatible with 1.0.0                 
    ✓ 1.2.0 must be compatible with 1.1.0

Ok, can you run this in a Node repl from your schemas/event/secondary clone and paste the output?

const jsonschemaTools = require('@wikimedia/jsonschema-tools');
const options = jsonschemaTools.readConfig();
const allSchemas = jsonschemaTools.findSchemasByTitleAndMajor(options);

const legacyTestSchemas1 = allSchemas['analytics/legacy/test']['1'];
const materializedSchemas = legacyTestSchemas1.filter(schemaInfo => !schemaInfo.current).filter(schemaInfo => schemaInfo.contentType === options.contentTypes[0]);

const orderOfVersions = materializedSchemas.map(s => s.version);
console.log(orderOfVersions);

I get:

[ '1.0.0', '1.1.0', '1.2.0' ]

Also, what version of @wikimedia/jsonschema-tools do you have installed? I've got 0.8.1.

Ok, can you run this in a Node repl from your schemas/event/secondary clone and paste the output?

[ '1.2.0', '1.0.0', '1.1.0' ]

Also, what version of @wikimedia/jsonschema-tools do you have installed? I've got 0.8.1.

~/wikimedia/event-secondary$ npm view @wikimedia/jsonschema-tools version
0.8.1

it starts very clearly at the patch linked above

And if you checkout a commit before 778a881737a1f9c845795cb41bc00200ed81b989, is the order returned different?

it starts very clearly at the patch linked above

And if you checkout a commit before 778a881737a1f9c845795cb41bc00200ed81b989, is the order returned different?

Sorry to leave this out: yes, before that commit the schema versions are listed in order:

[ '1.0.0', '1.1.0', '1.2.0' ]

Looking under the soil, maybe _.groupBy is causing the disorder? this comment suggests that the result of groupBy can be out of order.

FWIW,

npm view lodash version
4.17.20

Interesting. I have the same lodash version (but I'm running on MacOS).

In lib/jsonschema-tools.js on Line 996 in groupSchemasByTitleAndMajor, what happens if you add a sort after the _.groupBy, e.g:

schemaByTitleMajor[title] = _.groupBy(
    schemaInfosByTitle[title], info => semver.parse(info.version).major
).sort(schemaInfoCompare);

(schemaInfoCompare is defined above for sort order).

The compatibility tests assume that schemaInfo objects will be sorted by version number when iterating through them to check backwards compatibility. But there seems to be a bug in the schemaInfoCompare comparison function in jsonschema-tools that is for some reason causing an analytics/schema/test schema to sort to the top, out of expected order. (On my machine, it's 1.1.0 and not 1.2.0, but the end result is the same test failure.)

The bug seems to be in the treatment of schemas with titles containing the substring common. Commenting out that comparison logic like this fixes the test failure:

function schemaInfoCompare(infoA, infoB) {
    ...
    return // infoACommon === infoBCommon ? 0 : (infoACommon ? -1 : 1) ||
    ...
}

Alternatively, updating the schema compatibility test case to re-sort the schemas in a group immediately before checking BC will resolve the issue.

Actually, it's more than just the common stuff. Commenting out that part fixes this particular test failure, but it looks like there's still other weird stuff going on on the margins with that sorting logic. (This is with node 10.15.3 and jsonschema-tools 0.8.1 on Ubuntu, btw.)

_.groupBy returns an object so that exact patch didn't work, but I get the idea. Unfortunately, lodash doesn't support a custom comparator.

Here's an attempted fix: https://github.com/wikimedia/jsonschema-tools/pull/24 , but please note I haven't fully tested this locally.

@awight what about:


function groupSchemasByTitleAndMajor(schemaInfos) {
    const schemaInfosByTitle = groupSchemasByTitle(schemaInfos);

    const schemaByTitleMajor = {};
    _.keys(schemaInfosByTitle).forEach((title) => {
        const groupedByMajorVersion = _.groupBy(
            schemaInfosByTitle[title], info => semver.parse(info.version).major
        );

        // Make sure the schema versions in each major version are sorted properly.
        _.keys(groupedByMajorVersion).forEach(majorVersion => {
            schemaByTitleMajor[title][majorVersion] =
                groupedByMajorVersion[majorVersion].sort(schemaInfoCompare);
        });
    });
    return schemaByTitleMajor;
}

(Or another equivalent but more elegant solution :) )

fdans moved this task from Incoming to Event Platform on the Analytics board.

NIIICE THANK YOU FOR FINDING THAT!

Published as version 0.9.0 on npm.

awight claimed this task.