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

test: set up authentication #570

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

JeffreytheCoder
Copy link
Contributor

@JeffreytheCoder JeffreytheCoder commented Apr 14, 2024

What this PR does

  • Set up authentication for E2E tests. Simulate login flow using user credentials stored in environment variables. Run once before all test suites by saving auth file to storage state
  • Modified Playwright configuration to correctly run the tests
  • Added an example test to see if EC renders
  • Wrote README on how to set up tests

Acceptance Criteria fulfillment

  • login()

Fixes #546

Video/Screenshots

test-auth

@JeffreytheCoder
Copy link
Contributor Author

JeffreytheCoder commented Apr 14, 2024

I changed Playwright's base url to http://localhost:6006 to run tests against EC in storybook. @sidmohanty11 Do we have storybook running in workflow? We might need to make some changes

@Spiral-Memory
Copy link
Collaborator

Hey @JeffreytheCoder , I have some questions in mind. I don't have much understanding in setting up these tests, but I want to learn more about it so that I can also contribute by writing e2e tests, as the central issue is huge. I was going through your changes, and I would like to ask a few questions. It would be great if you could spare some of your time answering them and help me understand.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Apr 14, 2024

What each of these is doing and why did you keep 2 different ports for this? Earlier both of them were the same. After changing it to localhost:6000 and localhost:5173, I can see that Playwright tests are failing in your PR, and it says that it is unable to connect to that, and the connection is refused maybe because server is started on different port and you are trying to connect to a different port. However, before, it used to connect and have a check on basic things like whether the chat header is present or not, and it obviously happens if it is able to see the UI. I might be asking silly question, I am not exactly how it works properly, could you please help me understand it ?

image Screenshot 2024-04-14 104747 image

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Apr 14, 2024

I would like to know why you removed the host. Without a host, how will it connect to the Rocket.chat server? I believe chat.avitechlab.com was a hosted Rocket.Chat server on which api call could be made and tested on. Could you please help me understand this change?

image

@JeffreytheCoder
Copy link
Contributor Author

Hey @Spiral-Memory I was confused with whether the tests should run in development environment or in a dedicated testing environment in CI. I changed host to storybook and added environment variables so that each EC dev can run tests using his/her own server. However, if we only want to run tests during CI, I'll change it back to using chat.avitechlab.com.

@JeffreytheCoder
Copy link
Contributor Author

Before this PR, EC was set to anonymous mode for CI tests. I was thinking we can just discard the authentication setup, but then I saw EC currently only allows anonymous read, not anonymous write. If this is a feature, not a bug, we then need a test account of the hosted server to sign in and test the messaging features.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Apr 14, 2024

Hey @Spiral-Memory, I was confused about whether the tests should run in the development environment or in a dedicated testing environment in CI. I changed the host to Storybook and added environment variables so that each EC dev can run tests using their own server. However, if we only want to run tests during CI, I'll change it back to using chat.avitechlab.com.

In my opinion, tests should run on CI to achieve an automated workflow. Additionally, the environment variables for login are fine; the repository owner can add those environment variables to enable automated login on chat.avitechlab.com.

Also, we have two modes in RC. One is anonymous mode, which allows us to read messages if the workspace owner has enabled it. So, there's no need to remove that. Let's check for the anonymous mode first. Once the test passes, we can proceed to login and test the other features which require write access accordingly.

Also login is necessary to test other APIs that require auth, so we can't avoid login at all. anonymous mode is given by RC so that user can at least read messages without creating a account, and this is specific to workspace owner, they have the permission to enable / disable it and In EC, we need to test all scenarios, so testing both could be beneficial.

Regarding the test user, I think Abhinav has hosted that server so he or siddarth will add the credentials himself for test with admin access.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Apr 14, 2024

What each of these is doing and why did you keep 2 different ports for this? Earlier both of them were the same. After changing it to localhost:6000 and localhost:5173, I can see that Playwright tests are failing in your PR, and it says that it is unable to connect to that, and the connection is refused maybe because server is started on different port and you are trying to connect to a different port. However, before, it used to connect and have a check on basic things like whether the chat header is present or not, and it obviously happens if it is able to see the UI. I might be asking silly question, I am not exactly how it works properly, could you please help me understand it ?

image Screenshot 2024-04-14 104747 image

Also, once we refer to this, I think one is to start the EC, and the other part will go to that URL and test. So, keep it consistent, and it should be the same to successfully test things. Again, I'm not very sure how testing works, but this is what makes sense to me. Let's give it a try.

Also, For testing in your local system, use your Rocket chat server URL instead of chat.avitechlab.com and while raising PR, use this, once the maintainer add their credentials it will start working.

@JeffreytheCoder
Copy link
Contributor Author

JeffreytheCoder commented Apr 14, 2024

Hey @abhinavkrin @sidmohanty11 do you guys have the admin credentials to the test server chat.avitechlab.com?

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Apr 14, 2024

Hey @abhinavkrin @sidmohanty11 do you guys have the credentials to the test server chat.avitechlab.com?

Obviously they will have ig 😂, for testing, btw you can create your own account on that (avitech) Rocket.Chat server for now. I just created one for me. (but not with admin access obviously)

@JeffreytheCoder
Copy link
Contributor Author

Hey @abhinavkrin @sidmohanty11 do you guys have the credentials to the test server chat.avitechlab.com?

Obviously they will have ig 😂, for testing, btw you can create your own account on that (avitech) Rocket.Chat server for now. I just created one for me. (but not with admin access obviously)

Ohh that's great. I thought we need to share a test account so that's why I asked haha

@Spiral-Memory
Copy link
Collaborator

Hey @abhinavkrin @sidmohanty11 do you guys have the credentials to the test server chat.avitechlab.com?

Obviously they will have ig 😂, for testing, btw you can create your own account on that (avitech) Rocket.Chat server for now. I just created one for me. (but not with admin access obviously)

Ohh that's great. I thought we need to share a test account so that's why I asked haha

It depends on Abhinav.

Also, we might need to test some features with admin access, so I'm a little unsure if Abhinav will provide a test user with admin access. Haha... Maybe we can test non-admin related features with our own credentials on that server or using a common shared test user (without admin access / with admin access, depending on Abhinav's choice). But I think he is quite busy these days, so for now let's use our credentials to conduct testing on the local system (either with our local server (if admin access is required) or with the Avitechlab server).

@JeffreytheCoder
Copy link
Contributor Author

Anyway we need a shared account for CI to use. I created a tester account and we can change to an admin account later
Screenshot 2024-04-13 at 11 41 57 PM

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Apr 14, 2024

Yup ! Great thanks for sharing, but i think for CI, abhinav or siddarth will add an account with admin access in GitHub workflow credentials.

@Spiral-Memory
Copy link
Collaborator

Anyway, let's start working on it.. once the login is complete and once you make the required changes to this PR. Let me know, we will test together !!

@JeffreytheCoder
Copy link
Contributor Author

Added a new commit and the tests now work in CI :)

@Spiral-Memory
Copy link
Collaborator

@sidmohanty11 Could you once add credentials in GitHub workflow directly so that it can be fetched from. env rather than hardcoding a test user.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Apr 14, 2024

Added a new commit and the tests now work in CI :)

Great but you have removed some tests, like seeing the messages and stuff from the example.spec.ts, once revert those as well.

Also, after logging in, we need to check things like whether the input box is now enabled, whether the "Login successful" toast message is rendered, etc., to verify that the login has been successful, in my opinion. Please mark it as a draft for now, and once it's done, make it ready for review to avoid confusion.

@JeffreytheCoder JeffreytheCoder marked this pull request as draft April 14, 2024 15:06
@JeffreytheCoder
Copy link
Contributor Author

Added a new commit and the tests now work in CI :)

Great but you have removed some tests, like seeing the messages and stuff from the example.spec.ts, once revert those as well.

Also, after logging in, we need to check things like whether the input box is now enabled, whether the "Login successful" toast message is rendered, etc., to verify that the login has been successful, in my opinion. Please mark it as a draft for now, and once it's done, make it ready for review to avoid confusion.

Let's wait until the meeting to verify these changes

@Spiral-Memory Spiral-Memory mentioned this pull request Apr 14, 2024
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E Tests
2 participants