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

Fix browser crashing issues on workspace loader. #12421

Merged
merged 4 commits into from
Jan 28, 2019
Merged

Fix browser crashing issues on workspace loader. #12421

merged 4 commits into from
Jan 28, 2019

Conversation

monaka
Copy link
Member

@monaka monaka commented Jan 15, 2019

What does this PR do?

It fixes crashing web browsers by workspace-loader under unstable networks.

What issues does this PR fix or reference?

#11893 #12402 (and possibly #9295 #12063)

Note: I found similar bugs in Dashboard also. Issues above will be fixed after merged this and next PR.

@monaka
Use const instead of let if it can.
9367529 
Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
@che-bot
Copy link
Contributor

che-bot commented Jan 15, 2019

Can one of the admins verify this patch?

1 similar comment
@che-bot
Copy link
Contributor

che-bot commented Jan 15, 2019

Can one of the admins verify this patch?

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Jan 15, 2019
@vparfonov
Copy link
Contributor

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jan 15, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12421
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@monaka
Use updated authentication tokens always (refs #12042).
ce24bbe 
Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
@monaka
Wait for the reconnection established. (refs #11893)
0dac918 
Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
@monaka
Catch the unhandled Promise.reject().
645e2c0 
Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
@monaka
Copy link
Member Author

monaka commented Jan 16, 2019

I fixed all pointed. Could you review again to make sure?

vitaliy-guliy
vitaliy-guliy approved these changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy left a comment

Choose a reason for hiding this comment

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

Nice. Thanks for improvement!

artaleks9
artaleks9 previously requested changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@artaleks9 artaleks9 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I see the test report (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1427//Selenium_20tests_20report/) shows a big regression, only 24.476% tests success. So che-qa has to investigate what is the reason.

@artaleks9
Copy link
Contributor

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jan 16, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12421
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@artaleks9
Copy link
Contributor

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jan 16, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12421
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@monaka
Copy link
Member Author

monaka commented Jan 17, 2019

I checked both CI failed and test regressions. I can't believe they are caused by this PR. This doesn't touch Selenium and/or Theia.

@sleshchenko
Copy link
Member

sleshchenko commented Jan 17, 2019 •
edited
Loading

@monaka Thanks for your contribution. Can not wait until your PR will be merged.
For some reasons, Theia module build is not stable. It is not definitely your PR issue.
As about test regressions - I believe that they are not caused by your PR but we need to investigate cause and fix it whether it is CI infrastructure stability or seleniums tests.
Thanks again and sorry for issues you faced with ci-test.

@sleshchenko
Copy link
Member

ci-test

@monaka
Copy link
Member Author

monaka commented Jan 17, 2019

@sleshchenko No problem :) I just wrote my impression about logs. I'm making similar patches for dashboard now. I'll send it after this was merged.

@sleshchenko sleshchenko mentioned this pull request Jan 17, 2019
Closed
@che-bot
Copy link
Contributor

che-bot commented Jan 17, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12421
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@artaleks9
Copy link
Contributor

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jan 17, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12421
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@artaleks9 artaleks9 dismissed their stale review January 17, 2019 18:56

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1443/) doesn't show any regression against this Pull Request.

@artaleks9
Copy link
Contributor

Sorry again for issues you faced with ci-test.

@monaka
Copy link
Member Author

monaka commented Jan 18, 2019

ci-build

1 similar comment
@monaka
Copy link
Member Author

monaka commented Jan 19, 2019

ci-build

@ashumilova
Copy link
Contributor

ci-build

@vparfonov vparfonov merged commit f02d8af into eclipse-che:master Jan 28, 2019
@che-bot che-bot added this to the 6.18.0 milestone Jan 28, 2019
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 28, 2019
@monaka monaka deleted the pr-fix-network-issues-on-workspace-loader branch January 30, 2019 05:36
@monaka monaka mentioned this pull request Jan 30, 2019
Closed
@sleshchenko sleshchenko mentioned this pull request Feb 18, 2019
Closed
skabashnyuk pushed a commit to skabashnyuk/che that referenced this pull request Mar 11, 2020
@monaka @vparfonov
Fix browser crashing issues on workspace loader. (eclipse-che#12421)
81b8022 
* Use `const` instead of `let` if it can.

Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>

* Use updated authentication tokens always (refs eclipse-che#12042).

Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>

* Wait for the reconnection established. (refs eclipse-che#11893)

Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>

* Catch the unhandled Promise.reject().

Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vparfonov vparfonov vparfonov approved these changes

@vitaliy-guliy vitaliy-guliy vitaliy-guliy approved these changes

@artaleks9 artaleks9 artaleks9 left review comments

@akurinnoy akurinnoy Awaiting requested review from akurinnoy

Assignees
No one assigned
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Milestone
6.18.0
Development

Successfully merging this pull request may close these issues.
8 participants
@monaka @che-bot @vparfonov @artaleks9 @sleshchenko @ashumilova @vitaliy-guliy @benoitf