Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[GH-21322] - Migrate /enterprise/guest_accounts Tests to TS #11327

Closed
wants to merge 8 commits into from

Conversation

davidtaing
Copy link
Contributor

@davidtaing davidtaing commented Oct 11, 2022

Summary

Migrates the following integration tests to TS:

  • guest_add_spec
  • guest_experience_ui_spec
  • guest_feature_spec
  • guest_identification_spec
  • guest_identification_ui_not_cloud_spec
  • guest_identification_ui_spec
  • guest_invitatioin_ui_more_spec
  • guest_invitatioin_ui_spec
  • guest_popover_ui_spec
  • guest_removal_ui_spec
  • member_invitation_ui_spec
  • system_console_guest_access_ui_spec
  • system_console_guest_not_cloud_spec
  • system_console_manage_guest_spec

Updated Type Definitions:
/enterprise/elasticsearch_autocomplete/helpers/index.js

  • adds explicit createPrivate channel export due to typing being lost from default module.exports

extended_commands.d.ts

  • add missing typedWithForce function definition

email.d.ts

  • fix getRecentEmail param typedef

common.d.ts

  • add missing uiGetButton param definition

sidebar_left.ts

  • fix uiOpenTeamMenu item param optional

user.d.ts

  • add missing apiAdminLoginWithMFA param definition
  • made apiCreateUser options param optional
  • made apiCreateGuestUser options param optional

ui_commands.ts

  • change clickPostCommentIcon location param to optional

Ticket Link

mattermost/mattermost#21322

Related Pull Requests

N/A

Screenshots

Release Note


extended_commands.d.ts
- add missing typedWithForce function definition

email.d.ts
- fix getRecentEmail param typedef

common.d.ts
- add missing uiGetButton param definition

sidebar_left.ts
- fix uiOpenTeamMenu item param optional

user.d.ts
- add missing apiAdminLoginWithMFA param definition
- made apiCreateUser options param optional
- made apiCreateGuestUser options param optional

ui_commands.ts
- change clickPostCommentIcon location param to optional
due to module.exports being typed as any, resulting in
the error below.

Fixes Error Message in guest_add_spec.ts:
Module '"../elasticsearch_autocomplete/helpers"' has no exported member 'createPrivateChannel'.ts(2305)
Files moved to TS:
- guest_add_spec
- guest_experience_ui_spec
- guest_identification_spec
- guest_identification_ui_not_cloud_spec
- guest_identification_ui_spec
- guest_invitation_ui_more_spec
- guest_invitation_ui_spec
- guest_popover_ui_spec
- guest_removal_ui_spec
- member_invitation_ui_spec
- system_console_guest_access_ui_spec
- system_console_manage_guest_not_cloud_spec
- system_console_manage_guest_spec
Not too sure about putting an empty function there. Please provide
feedback on the best way to approach it.
@mm-cloud-bot
Copy link

@davidtaing: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mattermod
Copy link
Contributor

Hello @davidtaing,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermod mattermod requested review from a team and m1lt0n and removed request for a team October 11, 2022 15:33
@mattermod mattermod added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Contributor labels Oct 11, 2022
@mattermod
Copy link
Contributor

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

@davidtaing davidtaing changed the title Gh 21322 [GH-21322] - WIP Migrate /enterprise/guest_accounts Tests to TS Oct 11, 2022
e2e/cypress/tests/support/api/system.d.ts Outdated Show resolved Hide resolved
@@ -205,7 +205,7 @@ declare namespace Cypress {
* @example
* cy.apiCreateUser(options);
*/
apiCreateUser(options: Record<string, any>): Chainable<{user: UserProfile}>;
apiCreateUser(options?: Record<string, any>): Chainable<UserProfile>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving to Chainable<UserProfile> will probably break the other tests.

This was changed in PR #10910 - a.k.a the Support Migration PR

Copy link
Member

Choose a reason for hiding this comment

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

Right. It should match the actual implementation. Please update to latest master.

@M-ZubairAhmed M-ZubairAhmed requested review from furqanmlk and saturninoabril and removed request for m1lt0n October 11, 2022 16:02
@saturninoabril
Copy link
Member

Thanks @davidtaing! Please update to latest master then I'll take a look again. Ping whenever ready for review.

@davidtaing
Copy link
Contributor Author

That's great @saturninoabril! Thanks for the feedback.

I get this in motion when I get the chance later. ETA: ~4ish hours.

🙂

@davidtaing
Copy link
Contributor Author

davidtaing commented Oct 14, 2022

@saturninoabril, all done mate!

Edit: sorry, not done, I found a few errors via check-types. Fixing them now!

Edit 2: Now I should be done!

@davidtaing davidtaing changed the title [GH-21322] - WIP Migrate /enterprise/guest_accounts Tests to TS [GH-21322] - Migrate /enterprise/guest_accounts Tests to TS Oct 14, 2022
@furqanmlk
Copy link
Contributor

@davidtaing
Can you please fix lint errors ?

Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

@davidtaing thanks for updating! Several tests failed due to the changes made here. Before committing the changes, please make sure that the tests are passing on your local.

@davidtaing
Copy link
Contributor Author

Sorry, I will fix some of the linting errors and check the tests on my local before resubmitting.

I hope I didn't use up too much of your time.

@saturninoabril
Copy link
Member

No worries and thanks for your contribution!

@davidtaing
Copy link
Contributor Author

I'd like to unassign myself from this issue for the following two reasons:

  1. It looks like I need an enterprise license to run these tests for these files on my local machine.
  2. I had a nightmare of a time getting the dev environment setup on a new Ubuntu install.

But first things first, a sincere thank you for this opportunity. I have gained a lot of value from this PR, I have learnt a lot and you have made me a better Software Engineer. 👍

This was my gameplan:

  • Get the master branch server & webapp running on my local machine ✅
  • Get the unit tests running ✅
  • Get the integration tests running ❌
  • Checkout to my working branch
  • Run tests again & run linter and fix any issues
  • Submit

Note: I was running a fresh Ubuntu 22.04.1 install.

But $ make e2e-test was failing with "server didn’t start in 60 seconds"

Also all of my integration tests were failing when I ran $ npm run cypress:run in the e2e/cypress folder.

Digging through the Makefile for both server & webapp, I was able to build the story:
Failed: $ make e2e-test in webapp root folder
—> Calls: $ make test-data in the server root folder
—-> Calls: $ make inject-test-data - server
——> Calls: $ make test-data - server
——-> Calls: ./scripts/wait-for-system-start.sh- server
———> Calls: bin/mmctl system status --local - server
———-> Error: "socket file "/var/tmp/mattermost_local.socket" doesn't exists, please check the server configuration for local mode”

That error wasn't being propagated up from bin/mmctl to e2e-test.

Solution: Change the ServiceSettings.EnableLocalMode to true in the server/config/config.json file.
Source: mattermost/mattermost#20221 (comment)


Then I found e2e-test was resetting EnableLocalMode to false

mattermost-webapp/Makefile

Lines 124 to 126 in 0a85176

cd $(BUILD_SERVER_DIR) && [[ -f config/config.json ]] && \
cp config/config.json config/config-backup.json && make config-reset || \
echo "config.json not found" && make config-reset


Running $ npm run cypress:run in the e2e/cypress folder
Untitled

I'm assuming that 107 of the failed test suites were the enterprise tests.


I'd still like to contribute to Mattermost in the future and I'll be back, but this isn't a battle that I want to fight. Moving forward for all of my work in the future, I will be running tests and linting as part of my workflow.

@davidtaing
Copy link
Contributor Author

Just wanted to ask some stupid questions as well.

  • Is there a way to run Cypress in parallel on my local machine? I'm assuming it's single threaded.
  • And could I speed up the Cypress tests by running the Server on a different machine and hitting it over my LAN with the Webapp?

@davidtaing davidtaing closed this Oct 17, 2022
@furqanmlk
Copy link
Contributor

@davidtaing
Sorry to hear that you are having issue when running Enterprise test cases.
I don't think you need any Enterprise license to run these test cases.
To setup local development environment, we can have zoom call to discuss your questions.
Please let me know.

@furqanmlk furqanmlk reopened this Oct 18, 2022
@furqanmlk furqanmlk removed their request for review October 19, 2022 15:31
@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@lieut-data
Copy link
Member

Heads up that as part of our efforts to move to a monorepo, we're closing out a number of older pull requests like this one to help streamline the effort.

If you'd like to preserve these changes -- even if you're not the original author! -- feel free to resubmit the pull request against the monorepo once it's ready. You can subscribe to mattermost-server-issue-22420 for status updates on this effort.

@lieut-data lieut-data closed this Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants