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.

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

Update scheme tests #8835

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Update scheme tests #8835

wants to merge 8 commits into from

Conversation

deathaxe
Copy link
Contributor

This PR updates unittests used to verify channel.json and repository.json integrity.

Despite #8050 proposing use of separate github actions, this PR updates built-in scheme tests as a first step, so this repo can benefit from various improvements/fixes.


Remarks:

The test cases are currently designed primarily for use with this repo. Creating dedicated action runners therefore requires some changes such as addin parameters for repository files to check.

Most external repositories use packages.json which is not recognized by current tests.

This commit adds `Binary` and `Git Formats` to the list of default packages.
1. sort urls alphabetically
2. reformat patterns to use more of available space
3. disallow `#` (start of anchors) and `?` (start of parameters)
   in user and repository names
4. move (?<!\.git) to ensure not to accept something like
   https://github.com/repo/user.git/tree/master
Now that arm64 is supported it is very valid to have both x32 and x64
to specify arm64 is not supported.
To support validation and completions via LSP-json, Package Control ships
json schemas for channels and repositories.

This commit allows them to be explicitly assigned without tests failing via
- "$schema": "sublime://packagecontrol.io/schemas/channel",
- "$schema": "sublime://packagecontrol.io/schemas/repository",
Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

@braver
Copy link
Collaborator

braver commented Oct 25, 2023

Maybe @FichteFoll for an extra pair of eyes on this.

Bit if a shame there is all this code to implement validation but not a JSON schema? Maybe beyond the scope of this PR but I'd like to see a schema personally.

@FichteFoll
Copy link
Collaborator

Thanks for the mention. I'll take a look when I find the time, latest on the weekend.

@deathaxe
Copy link
Contributor Author

JSON schemas have been created at https://github.com/wbond/package_control/blob/four-point-oh/sublime-package.json for use in ST.

I also have them explicitly, locally.

@rchl
Copy link
Contributor

rchl commented Oct 25, 2023

Note that errors reported based on schema validation will likely not be as user-friendly as custom validation code and maybe sometimes even confusing.

@deathaxe
Copy link
Contributor Author

I second these concerns.

@braver
Copy link
Collaborator

braver commented Oct 26, 2023

Certainly, unless you're used to reading them that's very true. However, I would argue a schema is a more readable and reliable specification than any validation script. IMO one must have a schema and validate accordingly, one should have user friendly feedback.

@deathaxe
Copy link
Contributor Author

deathaxe commented Oct 26, 2023

I also found various pitfalls when writing the schemes, so whether they are more reliably is not for sure. It's just a question of declarative or command driven validation.

A vision would be do use pydantic to model the data and let it create the schemes and everything else - single source of truth.

@braver
Copy link
Collaborator

braver commented Oct 26, 2023

421286864750641155

url and base keys are already handled some lines above.
Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

... didn't match osx-arm64 before.
Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

@FichteFoll
Copy link
Collaborator

Thanks for the mention. I'll take a look when I find the time, latest on the weekend.

So, that obviously didn't happen and I'm not sure when the next time window for me to take a deeper look into this will be. If you three are satisfied with the solution (especially @braver, since he'll be using it for the most part), feel free to adopt it already. I can always check it out later and make suggestions afterwards.

As for the schema, don't forget that there are some things in the channel test suite that I don't think a schema supports, such as validating across multiple files that each package name (including previous names) only occurs once.

@braver
Copy link
Collaborator

braver commented Nov 19, 2023

I don't see anything weird in the diff from glancing over it. I don't really know how to run this locally for testing? But the approach is sound, so I'm also ok with just merging it as is.

such as validating across multiple files

Well you could merge all the different json files into one dataset first. I'm sure the script does things a schema can't do (or do easily), but the thing with scripts is that you can't easily describe what it does exactly so you're maybe just going by "it's 800+ lines, it must be pretty advanced". The opposite it likely to be true too, a schema might be able to do things that the script can't (or do it more easily). But maybe that's enough about schema's. This PR is a good step regardless of what we do later.

@deathaxe
Copy link
Contributor Author

This PR doesn't provide substantial changes in functionality. Just drops python 2 and fixes smaller glitches. It's an excerpt of changes made to four-point-oh branch. If it wasn't complete before, it is most likely not at the moment as well.

To run tests locally just open console in root of this repo's worktree and run python tests\test.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants