Include version 1.0 extensions in testing and build.py #9812
Conversation
validator/build.py
Outdated
| @@ -164,11 +164,11 @@ def GenValidatorProtoascii(out_dir): | |||
| assert re.match(r'^[a-zA-Z_\-0-9]+$', out_dir), 'bad out_dir: %s' % out_dir | |||
|
|
|||
| protoascii_segments = [open('validator-main.protoascii').read()] | |||
| extensions = glob.glob('extensions/*/0.1/validator-*.protoascii') | |||
| extensions = glob.glob('extensions/*/(0.1|1.0)/validator-*.protoascii') | |||
There was a problem hiding this comment.
can we use versions from the list instead. https://github.com/ampproject/amphtml/blob/master/build-system/extensions-versions-config.js
There was a problem hiding this comment.
I don't see what that would change since that file does not enumerate all versions. The protoascii should define which tag and extension is for which version.
There was a problem hiding this comment.
That's why I want to use config from that file. All use 0.1 by default. The only one that needs 0.1|1.0 is amp-sidebar.
I think 0.1|1.0doesn't make sense to upgraded elements. for example extensions/amp-sticky-ad/0.1/validator-*.protoascii exists, but the file is deprecated and should be built/tested no more.
There was a problem hiding this comment.
Even though amp-sticky-ad 0.1 is deprecated it is still valid and therefore must still be tested.
There was a problem hiding this comment.
I agree. but the function you're changing is to generate validator protoascii. Is that still needed? I think we should always use 1.0 version validator-amp-sticky-ad.protoascii, but test both 0.1 version validator-amp-sticky-ad.html and 1.0 version validator-amp-sticky-ad.html ? Please correct me.
There was a problem hiding this comment.
That's a design decision that I haven't heard the validator team discuss. One plan we have discussed is to introduce versioning to the tagspecs themselves and also pull the protoascii out of the versioned file but leave the appropriate validator test files with their appropriate versions.
There was a problem hiding this comment.
Like the idea to pull the protoascii out of the versioned file
validator/engine/validator_test.js
Outdated
| const testPath = path.join(extension, '0.1', 'test'); | ||
| if (isdir(path.join(root, testPath))) { | ||
| testSubdirs.push({root: root, subdir: testPath}); | ||
| const extensionVersions = ['0.1', '1.0']; |
There was a problem hiding this comment.
#9730 already made the change.
There was a problem hiding this comment.
Ah, didn't see that in the PR before. Will need to pull it in for internal systems.
There was a problem hiding this comment.
I'll also create another PR to add that to the validator light tests.
There was a problem hiding this comment.
Actually I'm going to change this back to being explicit about version numbers rather than just every directory within the extension name.
No description provided.