-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Stabilize Smoke Test #34203
Comments
@isidorn I've noticed the smoke test getting the contents of |
@joaomoreno great analysis. I have looked into debug failures and the last 2 are the same and as you already noticed they come from the fact that I am getting the contents of As for the first two failures screenshots would be great help, I noticed we have a screenshot step in builds but I am not sure how to access them. Also when running the smoketest locally with latest I constantly get the following failures |
@isidorn To prevent getting text from previously opened editors, we have this wait for editor to get focus. Let's see what is that condition is missing in waiting for that editor to focus. I would probably take a screenshot immediately after waiting for About those above tests which are failing, I had never seen them. Are you consistently reproducing them? |
@sandy081 yes those tests are constantly failing for me locally (I just pulled to get latest) I added more screenshots to check what is the problem. I have also increased the ping timeout from 400 to 600 ms which should prevent second failure. However the first failure comes from the node debug adapter that step in sometimes continues and this happens rarely. Maybe @weinand can comment more as I remember there was an issue about step actually doing a continue. |
Maybe there's another monaco-editor hidden somewhere.
Can we somehow get rid of the timeout altogether and just fire the ping once the debugger is ready for it? |
Ok I have
Now I triggered 3 linux build they should pass, but we shall see soon |
I thought about that too... but it means that others might still hit the bug... can't we nail down the wait criteria for the editor? |
The problem with the search criteria is that the name of the file is nowhere in the content of the editor. So we can not check the content with anything else. I believe nobody else is hitting this issue yet is due to the fact that most files are being open via quick open and this is one of the rare ones that we use some other action to open a file - debug configure. |
Ok for some reason the smoke test is still failing. Continuing investigation tomorrow morning |
I believe you can get this from the base API.
Interesting. The answer might be here. At the same time, this doesn't sound like a good criteria. @bpasero might be setting that Finally, it seems that you might just want to assert that |
Yup, check this out. I've hit a breakpoint when If there is nothing that would tell us the editor is there and ready, I suggest we introduce that something. A signal, which should be smoketest specific, like |
Introducing such a smoke test specific attribute makes sense to me. I browsed a bit through the code and this looks like the place once the editor has resolved its input. And the way that would float up to the container would be the same as the AriaLabel all the way up to the editor configuration here. Another way is instead of introducing a new label we could modify the aria label based on the fact if the editor is completely loaded or not. fyi @bpasero @joaomoreno yes I could use fs to get the content of the launch.json. Let me try that |
It seems like the failures yesterday were connected to something failing before debug. |
Ok, now I am reading the launch.json via @sandy081 From time to time I still get data loss failures, example https://monacotools.visualstudio.com/Monaco/_build/explorer?_a=summary&buildId=32387 |
From running the 8 test suits, now I see the following patterns
@sandy081 @joaomoreno any idea what is going on with all the non debug failures |
Ok I have updated debug to not have any timeouts at all. I also updated the smoke test such that it prints out the port when it is ready to be pinged, this way there is no randomness on the debug smoke test side. |
Editor focus thingy is very subtle. I share the same observation as @joaomoreno that we are hitting the point when
I am not sure there is something which we can depend on for the editor is ready. It looks like Arial label containing file name is set on monaco editor only when the accessibility is on. Will check with @alexandrudima and see if he can shed some light here. Not sure what is causing this time out in dataloss test. From screenshots I see that test ran successfully. Also I suspect some editor focus issues. I will inspect on non-debug related issues. Thanks @isidorn |
All 3 linux builds passed -> checking off debug in first post, seems to be good now. |
Talked to @bpasero and @alexandrudima on the editor focus issue. Looks like active tab showing old content is a real issue. But for the smoke tests, we would need much more reliable way to find if the editor is ready or not. For this @alexandrudima suggested to add an attribute I will trigger more builds to see if everything is good. |
Okay.. Here is some good news... We have green smoke builds. The last 20+ builds are green except one (failing due to slow market place queries) which is OK. I think we can make them part of our main builds. Good job guys @joaomoreno @isidorn 👍 |
@isidorn One of the last Linux builds still failed in Debug: https://monacotools.visualstudio.com/Monaco/_build/index?buildId=32459&_a=summary Together with @sandy081, we've gone over the debug test suite and added a bit more screenshots so we get an idea of what is happening there. We've also changed a few of the assertions done in the suite. And we've also fixed an issue with the screenshots overall which caused screenshots to be placed in the wrong folder name. Here's our changes: 4c26c77 We triggered many new builds, so let's wait for them to come back. |
@joaomoreno your changes look great to me, thanks |
@joaomoreno Above screenshot is not from the failing test in the build. Failing test in the build is
and the above screen shot is from the test `changes 'workbench.action.toggleSidebarPosition' command key binding and verifies it But both of them are same suite. Does both tests are failing for you? I cannot reproduce locally now because, I am getting different error in Preferences due to the message box Due to the message box, smoke test is not able to find the dirty file to save. |
Sorry, I have to update my npm packages. This could be due to that. |
It failed only once on my machine (where the screenshot came from). |
This could happen when the following happens
Not sure how you landed into that situation. But added a fix in Preferences to always add a trailing comma to prevent this. Otherwise, preferences tests are good locally |
Latest round of tests: https://monacotools.visualstudio.com/Monaco/_build/index?buildId=33116&_a=summary Windows fails in dataloss, debug and git:
Mac fails in dataloss: https://monacotools.visualstudio.com/Monaco/_build/index?buildId=33115&_a=summary
And both Linux runs are failing with a very strange error: https://monacotools.visualstudio.com/Monaco/_build/index?buildId=33118&_a=summary
|
Windows:
Mac:
Linux:
|
Issue 1: Why are we seeing that message box - We should assume that there should be any messages by default. Issue 2: Waiting for text content to write is not still reliable? Issue 3: Git check out might be taking time in the set up? |
Very interesting. That bar shouldn't be there but also just thought about how dangerous it is to tell Spectron to click a specific element: another might be on top. I'll make sure that update popup doesn't come up. Not sure about issue 2. Something must be up. I'll also investigate issue 3. |
08e6c5d should disable updates during the smoketest. Looking into issue 3. |
Issue 3 possibly related to electron-userland/spectron#219 |
Related to issue 3: d78d5da should kill all leftover running instances of Code before starting the smoketest. |
Getting very close. @sandy081 Macos just failed with
Here's the screenshot: |
This last failure happens due to some letters are switched/missing when typing in the editor. It is the same underlying issue behind the Dataloss occasional failures.
I also just saw this happening on Linux too! Investigating... |
Here's what we're hitting: https://bugs.chromium.org/p/chromium/issues/detail?id=522853 After some effort I was able to get our very own application logs (which I will soon make a part of the artifacts to create during a smoketest build). Right in the beginning there appeared this message:
I checked what this meant and the shared memory in Linux is represented by the
I've tried saturating that memory manually and, lo and behold, YES, THAT'S IT. The fix is to increase that shared memory level to something more than 64MB. Working on that. |
@isidorn @mjbvz Another message going over the debug toolbar... 🤕 Both Debug and Git are suffering from this. The amount of work to get this **** working is surreal.
|
@mjbvz Can we get an easy way out of this, not through the settings? |
It seems it was a one time thing... no idea what happened there... maybe no Internet? |
But #32908 is interesting... |
Nope... I was able to get dbus working inside the container, those I start to think the problem is really behind using Ubuntu 14.04 for the base container image: |
@joaomoreno Are you still running into the npm issue? If the issue is just that npm is not being picked up properly, try setting an explicit path with |
No, it seemed to have only happened once. |
The crash problem was related to both I'll investigate the editor problem. |
OK there are only two problems left as far as I know: The editor input issue, in which some characters appear delayed in the buffer. We might want to have programmatic access to the editor @alexandrudima as a workaround. This happens in Windows, Linux and macOS. And the preferences conflict save, which could be related to the previous one as it occurs also when the previous one does @sandy081. |
Fixed the editor issue by inputting text into it via:
Finally closing this since we merged the branch: db578b9 Thanks for the efforts @michelkaporin @sandy081 @isidorn! 🍻 |
The smoke test underwent massive invasive surgery. We've accomplished to
We still need to
Dataloss "before all" hook
sometimes fails in Linux @sandy081Multi Root "before all" hook
sometimes fail in macOS @sandy081The text was updated successfully, but these errors were encountered: