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.

Sign up for GitHub

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

Jump to bottom

Restart clangd when changing build config + a few UI test changes #3017

Merged
merged 4 commits into from
Oct 31, 2018
Merged

Restart clangd when changing build config + a few UI test changes #3017

merged 4 commits into from
Oct 31, 2018

Conversation

simark
Copy link
Contributor

@simark simark commented Sep 28, 2018 •
edited
Loading

See individual commits for details.

@akosyakov akosyakov mentioned this pull request Oct 1, 2018
Closed
@marcdumais-work
Copy link
Contributor

Travis failed, with the error we get when we forget to check-in yarn.lock.

@simark simark force-pushed the cpp-restart-clangd branch from 2090a84 to 0d88264 Compare October 3, 2018 13:37
@simark
Copy link
Contributor Author

simark commented Oct 3, 2018

Travis failed, with the error we get when we forget to check-in yarn.lock.

That's weird because I don't see changes to yarn.lock when I build. I updated the PR to add the header to utils.ts, we'll see.

@marcdumais-work
Copy link
Contributor

Travis passed this time

@simark simark force-pushed the cpp-restart-clangd branch from 0d88264 to 1f236eb Compare October 5, 2018 20:22
@marcdumais-work
Copy link
Contributor

That's weird because I don't see changes to yarn.lock when I build.

confirmed - no yarn.lock changes when I build locally and Travis passed

@simark simark force-pushed the cpp-restart-clangd branch from 1f236eb to 70bcfad Compare October 30, 2018 17:26
@vince-fugnitto vince-fugnitto self-requested a review October 30, 2018 18:28
@simark simark force-pushed the cpp-restart-clangd branch 2 times, most recently from 1b56422 to 6eeb943 Compare October 31, 2018 12:57
@vince-fugnitto
Copy link
Member

thanks @simark, I'll do another round of review

@simark
Copy link
Contributor Author

simark commented Oct 31, 2018

thanks @simark, I'll do another round of review

As you wish, but nothing changed except the triple-equals.

@vince-fugnitto
Copy link
Member

thanks @simark, I'll do another round of review

As you wish, but nothing changed except the triple-equals.

Sorry my first pass-through was just looking at the code to see anything obvious.
I do have two questions however:

  1. If we in the future remove the cpp-extension or other extensions and place it outside of the main repo, what will we do with the UI tests? Will they migrated as well?

@simark
Copy link
Contributor Author

simark commented Oct 31, 2018 

1. If we in the future remove the `cpp-extension` or other extensions and place it outside of the main repo, what will we do with the UI tests? Will they migrated as well?

Yes, I think they should be migrated. I think we would need to publish the UI test utils as an npm package, so we can use it from other repos.

@vince-fugnitto
Copy link
Member 

1. If we in the future remove the `cpp-extension` or other extensions and place it outside of the main repo, what will we do with the UI tests? Will they migrated as well?

Yes, I think they should be migrated. I think we would need to publish the UI test utils as an npm package, so we can use it from other repos.

thanks for clarifying for me, sounds good!

Simon Marchi added 4 commits October 31, 2018 10:12
Make some waitFor* functions use waitForVisible
facdc86 
I am sometimes seeing some random failures in the UI tests.  I believe
that this will help (or even if it doesn't help, I think it is right).

When a button is present on the left or right tab bar, but the
corresponding view is closed, the node for that view is still present in
the DOM.  For example, even when the Files view is collapsed, the #files
div is present in the DOM, it's just not visible.  Therefore, the check
waitForExist('#files') in waitForFilesView isn't right, as it won't wait
until the Files view is open (which is what I assume is the goal of that
function).

This patch replaces waitForExist with waitForVisible, to make sure we
wait for it to be visible.

I also changed the names to be clearer about what the functions do, for
example waitForFilesView becomes waitForFilesViewVisible.

Change-Id: I3ba1d3899fe161c7c14ce1de47dd450d2b0842e0
Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
Make it possible to pass process args when starting a backend
1f1cb56 
When doing some tests that require starting a backend (in particular the
UI tests, but not only them), we sometimes need to pass arguments to
that backend.  Right now, the only way is to modify process.argv, but
it's fragile because we don't know yet what is there in the first place
(it's the arguments to the test process).

This patch makes it possible to pass args to the backend start
function.  In the normal case, we pass process.argv, but in the tests,
we can pass an arbitrary array.

Change-Id: I2ed2308509dd476ab4d1506482075863fc4e84eb
Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
Run examples/browser UI tests using a temporary workspace
3503f14 
I am looking to add a UI test for some C/C++ features.  That will
require writing some files to the workspace and open them in the editor.
Right now, the workspace used for these tests seems to be based on the
content of ~/.theia/recentworkspace.json.  We obviously don't want to
start writing stuff in the user's real workspaces, and since we don't
know what we will find in there, it could make the tests unstable.

This patch makes the UI tests run with a temporary workspace, deleted
after the test run.

All the UI tests still run simultaneously using the same
workspace/backend, so there is still room for unpredictability.

Change-Id: I9c140daa774cc5f67f80fc10302f7b34efc057a6
Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
Restart clangd when changing build configuration
cf8e5b5 
Currently, when the user changes build configuration, we send a
DidChangeConfiguration notification to clangd.  This does not work in
the particular case where the user wants to go back to the default of
not using a specific build configuration.  There is no way (as of now)
to tell clangd that we don't want to use a specific
compile_commands.json anymore.

This patch works around it by restarting clangd when the user switches
config.  The new clangd will be initialized with the newly selected
build config.

The only way to test this end to end (with clangd involved) was to add a
UI test.  The test is skipped if clangd is not available, since we don't
expect everybody to have it installed.  If/when the cpp extension is
moved to its own repo, then we can make the test unconditional.

Change-Id: Id4e42ccd3b69a147a4c099e73440b8df00981e67
Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
@simark simark force-pushed the cpp-restart-clangd branch from 6eeb943 to cf8e5b5 Compare October 31, 2018 14:13
@marcdumais-work
Copy link
Contributor

@simark do you know if the AppVeyor failure might be related to the PR? It seems to be mostly passing, recently.

@marcdumais-work
Copy link
Contributor

@kittaakos can I trouble you, to restart the AppVeyor tests for this PR? Thanks!

@marcdumais-work
Copy link
Contributor

@simark confirmed that the AppVeyor failure is not related. It looks like a race condition in the @theia/process tests, similar on one he recently fixed in @theia/task.

marcdumais-work
marcdumais-work approved these changes Oct 31, 2018
View reviewed changes
Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @simark

@simark
Copy link
Contributor Author

simark commented Oct 31, 2018 •
edited
Loading

Thanks. Merge at your convenience (since I can't do it).

@simark
Copy link
Contributor Author

simark commented Oct 31, 2018

@simark confirmed that the AppVeyor failure is not related. It looks like a race condition in the @theia/process tests, similar on one he recently fixed in @theia/task.

The failure in the process tests is certainly not related to this PR.

I had a theory about it (onExit handler being registered too late), but I don't think that can happen, so I don't really have a clue anymore.

@marcdumais-work
Copy link
Contributor

AppVeyor failed in another component this time. Looking, I found the same failure we had before in the tests for a different PR: https://ci.appveyor.com/project/kittaakos/theia/builds/19949944#L1527

Merging.

@marcdumais-work marcdumais-work merged commit 7d4fcad into master Oct 31, 2018
@marcdumais-work marcdumais-work deleted the cpp-restart-clangd branch October 31, 2018 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@marcdumais-work marcdumais-work marcdumais-work approved these changes

@paul-marechal paul-marechal Awaiting requested review from paul-marechal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simark @marcdumais-work @vince-fugnitto