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

NbLaunchDelegate fix and test multi line texts in suite.py #7011

Merged
merged 2 commits into from
Feb 11, 2024

Conversation

jtulach
Copy link
Contributor

@jtulach jtulach commented Jan 28, 2024

I cannot open graal/sdk project anymore. Turns out that now its suite.py is using multiline texts and the conversion of suite.py to JSON fails to convert that properly. This PR provides a test and a fix to handle such conversion properly. With the fix I am able to open sdk as well as truffle project without errors.

This fix affects NetBeans IDE, VSCode extension and IGV. CCing @rschatz, @gilles-duboscq

@jtulach jtulach self-assigned this Jan 28, 2024
@jtulach jtulach changed the title Support multi line texts in suite.py Support multi line texts in suite.py & NbLaunchDelegate fix Jan 30, 2024
assertEquals(fo, lkp.lookup(FileObject.class));

DataObject obj = lkp.lookup(DataObject.class);
assertNotNull("DataObject also found", obj);
Copy link
Contributor Author

@jtulach jtulach Jan 30, 2024

Choose a reason for hiding this comment

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

While writing my Enso code, I was surprised I cannot find EnsoDataObject in the lookup. 8838cff fixes that.

@jtulach jtulach marked this pull request as draft February 1, 2024 06:58
@jtulach jtulach changed the title Support multi line texts in suite.py & NbLaunchDelegate fix NbLaunchDelegate fix and test multi line texts in suite.py Feb 11, 2024
@jtulach jtulach marked this pull request as ready for review February 11, 2024 06:04
@mbien
Copy link
Member

mbien commented Feb 11, 2024

this looks like it needs the LSP [ci] enable Language Server Protocol tests and MX [ci] enable "build tools" tests tests?

@mbien mbien added Need Squashing LSP [ci] enable Language Server Protocol tests MX [ci] enable "build tools" tests labels Feb 11, 2024
@jtulach jtulach added the VSCode Extension [ci] enable VSCode Extension tests label Feb 11, 2024
@jtulach jtulach merged commit 3e15eae into apache:master Feb 11, 2024
4 checks passed
@mbien
Copy link
Member

mbien commented Feb 11, 2024

This change is currently breaking master.

Looking at the timestamps this PR was merged without waiting for CI to complete. Please check if this can be fixed.

https://github.com/apache/netbeans/actions/runs/7864172928 PR run
https://github.com/apache/netbeans/actions/runs/7864179478 master run

@jtulach
Copy link
Contributor Author

jtulach commented Feb 13, 2024

This change is currently breaking master.

Ops. Sorry for the mistake.

Looking at the timestamps this PR was merged without waiting for CI to complete. Please check if this can be fixed.

I guess I got confused with CI & labels. Everything was green,

image

but I probably used labels incorrectly. I'll do my best to be more careful next time.

@neilcsmith-net
Copy link
Member

Thanks for merging the revert @jtulach

Two things to be aware :

  1. When you force pushed to this it started a new set of tests based on the labels that had been set. These had not completed when this was merged.
  2. Adding labels by itself does not trigger tests to run again, unfortunately. Pushing to a PR does. And the "hidden" switch to rerun tests with new labels is to lock and unlock conversation (in case you're wondering about why I did this on ActionProvider and its Lookup in NbLaunchDelegate #7059 )

@mbien
Copy link
Member

mbien commented Feb 13, 2024

this page on the wiki is explaining a bit how the CI pipeline works:
https://cwiki.apache.org/confluence/display/NETBEANS/PRs+and+You+-+A+reviewer+Guide
the "Important" section mentions some details how the labels are taken into account and when the workflow sees changes.

I think this PR might have been fine if CI would have finished running after the second sync. There is actually an option to enforce "green checks" before gh allows the merge. But last time when we considered to enable that, there were concerns that this could lock us out as far as I remember. (although I think there would be enough tricks left to get around that lockout and unblock it again)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP [ci] enable Language Server Protocol tests MX [ci] enable "build tools" tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants