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

Fixed flaky test #16

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Fixed flaky test #16

merged 2 commits into from
Jun 23, 2023

Conversation

mauricioszabo
Copy link

So... this is the stupidest shit ever, but let's go:

Snippets package basically have no way of knowing if a package is "core" or not. So they actually do something weird - the last package that gets activated is the one that basically gets to define the snippet, in case of conflicting snippets keys.

But that's the problem: package activation is async - meaning that the order is not guaranteed. So the way the package decides if an user-installed package will have the snippet, is to sort packages by their directory - if a package contains node_modules on their path, it'll be considered a "core" package and it'll be sorted lower (I wish I was making this up, but unfortunately, I am not).

Which brings us to the worst problem so far: when language-javascript was not inside the packages directory, things were working just fine. Now... well, they are not - so Pulsar, on the CI, checks that language-javascript is inside the packages directory, and it uses that path instead of the one that's inside node_modules (why? Again, no idea - it shouldn't do that, honestly).

So this PR fixes this issue by patching the package to have a different directory.

@savetheclocktower
Copy link
Sponsor

I had thought that this was only a problem when I was developing locally — I ran into this whenever I had packages pinned in dev mode. I had not noticed that it ever happened on CI, so I didn't think it needed fixing.

But, yes, if this is affecting CI, then it needs to be fixed. This PR is fine, but eventually we'll need a better heuristic for testing if a package is core or not.

@DeeDeeG
Copy link
Member

DeeDeeG commented Jun 22, 2023

we'll need a better heuristic for testing if a package is core or not.

If we update the packageDependencies section of package.json to be fully up to date, we can read the list from there. Pretty sure that's what that's for.

@mauricioszabo
Copy link
Author

@DeeDeeG the problem is that package.json is inside Pulsar, inside the ASAR file, when packaged; this package is trying to read this info in user-space (package space?).

We might need an API to solve this

@savetheclocktower
Copy link
Sponsor

We might need an API to solve this

It should know the exact names of builtin packages, and should work even when it's not explicitly loading from the expected locations on disk. If I do ppm link to point language-javascript to a different location on disk, I still want it to be treated like a built-in package for nearly all purposes, including snippet evaluation order.

@mauricioszabo mauricioszabo merged commit 1717027 into master Jun 23, 2023
6 checks passed
@mauricioszabo mauricioszabo deleted the fix-flaky-test branch June 23, 2023 19:16
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.

None yet

3 participants