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

Uncomment and fix a settings-view package test #366

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Feb 1, 2023

A quick follow-up to #307.

What this fix is all about (click to expand):

There is a special handling of internal/bundled packages which specify their repository field of package.json as the exact GitHub repo URL of the main editor...

The editor (specifically some logic in the settings-view package, I guess) understands that bundled packages with "repository": "https://github.com/pulsar-edit/pulsar" should link to this repo's packages/[package name here] subdir, not just to the root repo URL as specified verbatim in the package's package.json, i.e. https://github.com/pulsar-edit/pulsar

This updates the dummy spec package used for this spec to have its repository URL be https://github.com/pulsar-edit/pulsar instead of the old https://github.com/atom/atom. Which, after this change, then matches with the editor's own repository URL in its own package.json. So the test, which assumes the special handling described above is in play, works as intended again and passes.


So this fixes a test that been failing and which got commented out during #307. (The fix is essentially a tiny re-branding task.)

This gives us one more test running, one more test passing. Not a huge deal, but feels better than just commenting this one out, and I got the fix sorted relatively quickly, so here we are. Figured I may as well submit this as a PR.

@DeeDeeG DeeDeeG changed the title Uncomment and fix settings view test Uncomment and fix a settings-view package test Feb 1, 2023
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Looks like an easy approval from me! More passing tests and a small change, couldn't be more awesome

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 2, 2023

Thank you for the review!

Merging because: Very very small fix, it works for me locally, and now in CI as well, and I confirm editing it to be wrong again (by typoing the repo name) makes it fail again, so that proves it's not a false-positive... And now I have an approve. Merge time!

@DeeDeeG DeeDeeG merged commit 221fc27 into master Feb 2, 2023
@DeeDeeG DeeDeeG deleted the uncomment-and-fix-settings-view-test branch February 4, 2023 03:33
@kaosine kaosine restored the uncomment-and-fix-settings-view-test branch February 5, 2023 00:23
@Spiker985 Spiker985 deleted the uncomment-and-fix-settings-view-test branch February 24, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants