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

Create tests for the "Typescript" and "Node-debug2" plugins #19474

Merged
merged 9 commits into from
Apr 1, 2021
Merged

Create tests for the "Typescript" and "Node-debug2" plugins #19474

merged 9 commits into from
Apr 1, 2021

Conversation

Ohrimenko1988
Copy link
Contributor

@Ohrimenko1988 Ohrimenko1988 commented Mar 31, 2021 •
edited
Loading

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Mar 31, 2021
@Ohrimenko1988 Ohrimenko1988 requested a review from sparkoo as a code owner March 31, 2021 19:11
@che-bot
Copy link
Contributor

che-bot commented Mar 31, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@sparkoo sparkoo removed their request for review March 31, 2021 20:28
Katka92
Katka92 reviewed Apr 1, 2021
View reviewed changes
tests/e2e/utils/BrowserTabsUtil.ts
import { Logger } from './Logger';

@injectable()
export class BrowserTabsUtil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class is mirroring some behavior from DriverHelper and is actually duplicating some of the methods there.
As the DriverHelper class is quite long, I'm +1 for moving all Tabs related functions here - like for example those methods:

  • getCurrentUrl
  • waitURL
  • navigateAndWaitToUrl
  • navigateToUrl
  • reloadPage
  • maximize

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is good but doesn't look so easy, I guess a lot of tests should be updated. We can create an issue for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I skimmed the usage of those methods, I don't think it's something taking more than half a day/one day of work (changes + some basic testing) so I would prefer to have that done together in one PR. If you're not willing to do that now, then please create the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more in line of marking the DriverHelper methods as deprecated in this PR and removing them in few released in separate PR.

Copy link
Contributor Author

@Ohrimenko1988 Ohrimenko1988 Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As I skimmed the usage of those methods, I don't think it's something taking more than half a day/one day of work (changes + some basic testing) so I would prefer to have that done together in one PR. If you're not willing to do that now, then please create the issue.

I strongly disagree with that.
I don't have a day for spending it with such issue with low priority.
I have a lot of tasks with much highest priority.

Copy link
Contributor Author

@Ohrimenko1988 Ohrimenko1988 Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested methods marked as "@deprecated".
The dedicated issue for refactoring created: #19484

@Ohrimenko1988
Add deprecated methods
82e90d0 
Signed-off-by: Ihor Okhrimenko <[email protected]>
@Ohrimenko1988
Apply lint changes
87300af 
Signed-off-by: Ihor Okhrimenko <[email protected]>
@che-bot
Copy link
Contributor

che-bot commented Apr 1, 2021

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@che-bot
Copy link
Contributor

che-bot commented Apr 1, 2021

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@Ohrimenko1988
Copy link
Contributor Author

[crw-ci-test --rebuild]

@che-bot
Copy link
Contributor

che-bot commented Apr 1, 2021

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@che-bot
Copy link
Contributor

che-bot commented Apr 1, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@Ohrimenko1988 Ohrimenko1988 merged commit f6e8a38 into eclipse-che:master Apr 1, 2021
@Ohrimenko1988 Ohrimenko1988 deleted the typescript-plugin branch April 1, 2021 18:07
This was referenced Apr 1, 2021
Closed
Closed
@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 Apr 1, 2021
@che-bot che-bot added this to the 7.29 milestone Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Katka92 Katka92 Katka92 left review comments

@rhopp rhopp rhopp approved these changes

@musienko-maxim musienko-maxim Awaiting requested review from musienko-maxim

@nickboldt nickboldt Awaiting requested review from nickboldt

Assignees
No one assigned
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Milestone
7.29
Development

Successfully merging this pull request may close these issues.
4 participants
@Ohrimenko1988 @che-bot @rhopp @Katka92