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

Update project and implement select tests #7

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

haydar-metin
Copy link
Contributor

@haydar-metin haydar-metin commented Feb 9, 2024

Description

This PR brings some necessary changes for the future:

  1. Updates the used frameworks and introduces fixes to work with them:
  • Theia-Playwright
  • GLSP-Client
  • Playwright
  1. Restructures example tests to be more align with GLSP-Client project structure
  2. Make GLSP-Playwright more defensive (e.g., more information, checks, vscode-setup)

Finally, test cases based for select are implemented.

What it does

  • Update folder structure to be similar to glsp-client
  • Remove aliasing from example project
  • Fix failing tests due to ghost elements

New Features

  • skipIntegration to skip specific integrations
  • Introduce * type for models to allow for all types
  • Make access more defensive

How to test

Please prepare the project by reading the README

  1. Run the tests as described there.

The standalone version succeeds all the thests.

Theia and VSCode fail the test:

    [...] › ../../tests/features/select/select.spec.ts:84:9 › The select feature › should allow to deselect a single element through a keybinding 

It seems that using Escape does not reset the select state.

Follow-ups

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)
New way of creation
  • GLSPApp

GLSP-Playwright follows a similar approach as Theia-Playwright. However, we do not require any loader. That means we can create our *App now directly.

Before

app = await GLSPApp.loadApp(WorkflowApp, {
    type: 'integration',
    integration
});

Now

app = new WorkflowApp({
    type: 'integration',
    integration
});
  • All integrations take now a IntegrationArgs as first constructor parameter

Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks for the update!
Code changes look good to me, I just have a few minor comments/suggestions.

However, there seems to be an issue with one of the newly added select test cases.
For me, in both the Theia and the VS Code integration the test The select feature- should allow to deselect a single element through a keybinding fails with a timeout.

Here is the error stack from Theia testing:

Running 43 tests using 6 workers
  1) [theia] › ../../tests/features/select/select.spec.ts:84:9 › The select feature › should allow to deselect a single element through a keybinding 

    Test timeout of 30000ms exceeded.

    Error: locator.waitFor: Test ended.
    Call log:
      - waiting for locator('body').locator('div.sprotty').locator('[data-svg-metadata-type="graph"]').locator('svg.sprotty-graph').locator('.selected').first() to be detached
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>
      -   locator resolved to visible <g id="workflow-diagram_0_task0" transform="transla…>…</g>


      94 |         await page.keyboard.press('Escape');
      95 |
    > 96 |         await Promise.all((await graph.locate().locator(`.${Selectable.CSS}`).all()).map(l => l.waitFor({ state: 'detached' })));
         |                                                                                                 ^
      97 |
      98 |         const after = await graph.getModelElementsBySelector(`.${Selectable.CSS}`, PModelElement);
      99 |         expect(after).toHaveLength(0);

        at map (/home/tobias/Git/OpenSource/glsp/glsp-playwright/examples/workflow-test/tests/features/select/select.spec.ts:96:97)
        at /home/tobias/Git/OpenSource/glsp/glsp-playwright/examples/workflow-test/tests/features/select/select.spec.ts:96:86

  1 failed
    [theia] › ../../tests/features/select/select.spec.ts:84:9 › The select feature › should allow to deselect a single element through a keybinding 

examples/workflow-test/package.json Outdated Show resolved Hide resolved
packages/glsp-playwright/package.json Show resolved Hide resolved
- Update folder structure to be similar to glsp-client
- Remove aliasing from example project
- Make access more defensive
- Introduce * type for models to allow for all types
- Fix failing tests due to ghost elements
- Update theia playwright framework
@haydar-metin
Copy link
Contributor Author

@tortmayr:

Both of the tests fail because:

[...] › ../../tests/features/select/select.spec.ts:84:9 › The select feature › should allow to deselect a single element through a keybinding
It seems that using Escape does not reset the select state.

So, both of Theia and VSCode do not clear the selected state if the escape is clicked multiple times. If I remember correctly, that was implemented with the accessibility features.

@tortmayr
Copy link
Contributor

@tortmayr:

Both of the tests fail because:

[...] › ../../tests/features/select/select.spec.ts:84:9 › The select feature › should allow to deselect a single element through a keybinding
It seems that using Escape does not reset the select state.

So, both of Theia and VSCode do not clear the selected state if the escape is clicked multiple times. If I remember correctly, that was implemented with the accessibility features.

I see, if this behavior is part of the a11y feature we should deactivate the corresponding test for now.
This feaute is currently only active in the standalon workflow example as support for Theia/VSCode is not yet implemented.
See (eclipse-glsp/glsp#1130 and eclipse-glsp/glsp#1129)

…d allow to skip tests based on running integration
@haydar-metin
Copy link
Contributor Author

  • The failing tests will be now skipped for Theia / VSCode.
  • With the env GLSP_SERVER_PLAYWRIGHT_MANAGED we can now define that the server will be started externally.
  • Fixed also the error notification triggered by Playwright by extracting the webserver and projects part of the config into custom files.

@tortmayr tortmayr self-requested a review February 22, 2024 15:36
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks! Changes look good to me and should make it easier to setup
CI workflows for UI tests in the respective GLSP components repositories.
Nice job!

@tortmayr tortmayr merged commit 3e54bf5 into eclipse-glsp:main Feb 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants