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

yeet earlier this.keymaps.loadBundledKeymaps call #464

Closed
wants to merge 1 commit into from
Closed

Conversation

meadowsys
Copy link
Member

@meadowsys meadowsys commented Apr 6, 2023

This is called once in the constructor, then again in initialize. The constructor call was too early, and i18n.t had not been available yet, but it was used indirectly by loadBundledKeymaps, causing stuff to crash.

PS. PAY ATTENTION TO CI TESTS

This is called once in the constructor, then again in `initialize`. The constructor call was too early, and `i18n.t` had not been available yet, but it was used by `loadBundledKeymaps`.
@Spiker985
Copy link
Member

Looks like as of right now (with ~10 packages still testing)

@confused-Techie
Copy link
Member

@Spiker985 @meadowsys
I should mention that the symbols-view failures might be due to something else.

I've noticed the two additional failures over on #453 & #452

@confused-Techie
Copy link
Member

Also just confirmed, looks like the earliest the failures of symbols-view appear (the 2 additional failures that is) is in #435 so second mate caused them, so don't hold them accountable here. But otherwise the others still apply of course

@meadowsys
Copy link
Member Author

slightly off topic, are the expected failures kept track of somewhere?

@Spiker985
Copy link
Member

slightly off topic, are the expected failures kept track of somewhere?

Tests channel in the discord is where confused has been pinning them

@Spiker985
Copy link
Member

Spiker985 commented Apr 6, 2023

Looks like the editor tests are running very long (I did just cancel their runs). It also looks like the MenuManager specs need updating

When the initial localization support was added, were the specs validated to make sure they were returning properly?

I also saw another t is not a function inside that spaghetti mess 🤔

@confused-Techie
Copy link
Member

@Spiker985 I've been trying to solve an issue that I thought was only on one of my PRs of the Editor tests running very very long.

I believe @meadowsys mentioned they had seen it before and simply rerunning the tests was able to get it working.

Pretty sure my failed attempts to resolve this are in #424

@confused-Techie
Copy link
Member

@Spiker985 yeah I'm seeing this in the editor tests

"Uncaught (in promise) TypeError: Cannot read property 't' of undefined", source: /home/runner/work/pulsar/pulsar/src/menu-manager.js (244)

But otherwise looks like the grammar tests, and extra failures elsewhere are now complaining about the path so improvement, we got new error messages

@Spiker985
Copy link
Member

Looks like it's a renderer crash that causes the test to restart ad infinitum

You can see it on my PR for bumping the editor test GHA

@confused-Techie
Copy link
Member

@Spiker985 Great find, looking further at the same test through our history

image

Doesn't look promising, especially in that first failing test run pictured I'm seeing this Renderer process crashed - see https://www.electronjs.org/docs/tutorial/application-debugging for potential debugging information. Renderer process crashed, exiting.

So it looks like maybe that PR has introduced some edge case causing a crash? But good to at least know what's causing the editor tests to be wonky, brings up a good point that we should probably also have a retry limit on these tests

@meadowsys
Copy link
Member Author

"Uncaught (in promise) TypeError: Cannot read property 't' of undefined", source: /home/runner/work/pulsar/pulsar/src/menu-manager.js (244)

Looks like might be another "use before initialise" type of bug. this line here in menu-manager.js is the culprit i think? On second thought, that's just registering an event handler, so I'm not sure if this is the problem, but i'll post it anyways. Maybe the event fires too early, and i18n hasn't / is still being initialised or something? hm,

@meadowsys
Copy link
Member Author

I18n on master was reverted, going to remake the i18n PR and fix the bugs there (including this one) before it gets merged again

@meadowsys meadowsys closed this Apr 6, 2023
@confused-Techie confused-Techie mentioned this pull request Apr 6, 2023
@meadowsys meadowsys deleted the fix-t branch September 12, 2023 06:36
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.

3 participants