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

Implement C/C++ devfile test #18009

Conversation

ScrewTSW
Copy link
Member

@ScrewTSW ScrewTSW commented Oct 1, 2020

What does this PR do?

  • Implements tests for new C/C++ devfile

What issues does this PR fix or reference?

#17709

How to test this PR?

  • Go into Eclipse repository che/tests/e2e
  • Run npm i and verify that all packages install without errors
  • Run tsc and verify that no errors are present
  • Export following variables:
    • TS_SELENIUM_BASE_URL
    • TS_SELENIUM_MULTIUSER=true
    • NODE_TLS_REJECT_UNAUTHORIZED=0
    • TS_SELENIUM_LOG_LEVEL="TRACE"
    • TS_SELENIUM_USERNAME
    • TS_SELENIUM_PASSWORD
  • Run npm run test-java-vertx to execute the smoke test devfile and verify that the tests are working correctly or run entire suite using npm run test-all-devfiles
  • Tests should pass without a failure (you may encounter stop and delete workspace failures if not using ephemeral mode by default)

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@ScrewTSW ScrewTSW added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. team/che-qe area/qe labels Oct 1, 2020
@ScrewTSW ScrewTSW changed the title 17709 feature implement cslashcplusplus devfile test 17709 Implement C/C++ devfile test Oct 1, 2020
@che-bot
Copy link
Contributor

che-bot commented Oct 1, 2020

❌ 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.

@ScrewTSW
Copy link
Member Author

ScrewTSW commented Oct 1, 2020

[crw-ci-test]

@che-bot
Copy link
Contributor

che-bot commented Oct 1, 2020

❌ 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.

@ScrewTSW ScrewTSW force-pushed the 17709-feature-implement-cslashcplusplus-devfile-test branch from 168540e to b23ba2a Compare October 1, 2020 08:34
@che-bot
Copy link
Contributor

che-bot commented Oct 1, 2020

✅ E2E Happy path tests succeed 🎉

See Details

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

commonLsTests.errorHighlighting(fileName, 'error;\n', 42);
// commonLsTests.codeNavigation(fileName, 42, 10, 'flag.go'); // codenavigation is inconsistent https://github.com/eclipse/che/issues/16929
// commonLsTests.codeNavigationGoTo(fileName, 42, 10, 'flag.go'); // codenavigation is inconsistent https://github.com/eclipse/che/issues/16929
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be fixed and uncommented by you're new functionality?


export function codeNavigation(openedFile: string, line: number, char: number, codeNavigationClassName: string) {
test('Codenavigation', async () => {
export function codeNavigationGoTo(openedFile: string, line: number, char: number, codeNavigationClassName: string, keyCombination: string = Key.chord(Key.CONTROL, Key.F12)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if GoTo is adding any value here - the codeNavigation already says that you should be navigated somewhere. Maybe it would be worth adding if it should redirect you to implementation, definition, references...

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to prepare it for the two approaches: codeNavigationGoTo and codeNavigationPeek as is explained in this issue: #17995

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so what about renaming this method to codeNavigationGoToImplementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily, because based on what key combination we send, we can go do Definition, Implementation, References, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

GoTo - we expect it to open a new file
Peek - we expect it to show the in-editor peek window

@@ -46,7 +46,7 @@ suite(`${workspaceStack} test`, async () => {
projectManager.openFile(fileFolderPath, fileName);
});

suite('Test golang example', async () => {
suite.skip('Test golang example', async () => { // test always fails, because the task does not show a notification about return code
Copy link
Contributor

@Katka92 Katka92 Oct 1, 2020

Choose a reason for hiding this comment

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

I'm not sure about skipping this test. In 7.19.x the notification was removed in all stacks so we would need to switch to another approach. I prefer to leave it unskipped.
issue for fix: #17973 (comment)
upstream PR with the change: eclipse-theia/theia#8331
blocker for implementing the fix: #17978

Copy link
Member Author

Choose a reason for hiding this comment

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

but if it will be unskipped, it will be causing test failures

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, but then it means that it should be skipped in all the devfile tests - I don't see much value in skipping that just in one of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will test it on latest che and skip where necessary

Copy link
Contributor

@Katka92 Katka92 left a comment

Choose a reason for hiding this comment

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

Please revert the skipping of test for command in Go. Others are just minor items.

@ScrewTSW ScrewTSW force-pushed the 17709-feature-implement-cslashcplusplus-devfile-test branch from b23ba2a to c7854b0 Compare October 5, 2020 08:27
@che-bot
Copy link
Contributor

che-bot commented Oct 5, 2020

✅ E2E Happy path tests succeed 🎉

See Details

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

@ScrewTSW ScrewTSW changed the title 17709 Implement C/C++ devfile test WIP: Implement C/C++ devfile test Oct 5, 2020
@ScrewTSW ScrewTSW force-pushed the 17709-feature-implement-cslashcplusplus-devfile-test branch from c7854b0 to 25f9681 Compare October 6, 2020 11:18
@ScrewTSW
Copy link
Member Author

ScrewTSW commented Oct 6, 2020

I've split the current PR into only C/C++ implementation and improvements that can be found here master...ScrewTSW:local_optimizing_devfile_tests

Signed-off-by: Tibor Dancs <tdancs@redhat.com>
@ScrewTSW ScrewTSW force-pushed the 17709-feature-implement-cslashcplusplus-devfile-test branch from 25f9681 to 16b6adb Compare October 6, 2020 11:29
@ScrewTSW ScrewTSW changed the title WIP: Implement C/C++ devfile test Implement C/C++ devfile test Oct 6, 2020
@che-bot
Copy link
Contributor

che-bot commented Oct 6, 2020

✅ E2E Happy path tests succeed 🎉

See Details

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

@ScrewTSW ScrewTSW merged commit 286a222 into eclipse-che:master Oct 7, 2020
@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 Oct 7, 2020
@che-bot che-bot added this to the 7.20 milestone Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/qe kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants