SWC-6621 #5250

hallieswan · 2024-01-06T00:21:52Z

lint e2e tests
fix errors raised by linter
use types from @sage-bionetworks/synapse-types

hallieswan · 2024-01-06T00:28:28Z

.eslintrc.js

+    '@typescript-eslint/no-inferrable-types': 'off',
+    '@typescript-eslint/ban-ts-comment': 'warn',
+    '@typescript-eslint/no-empty-interface': 'warn',
+    '@typescript-eslint/no-floating-promises': 'error',


For the most part, these rules are a direct subset of rules in synapse-web-monorepo. However, @typescript-eslint/no-floating-promises is set to error to catch issues in Playwright tests, as recommended in their best practices guide.

Do you think we should create a ticket to enable this rule in synapse-web-monorepo? I think it makes sense since we're writing tests in essentially the same way

Agreed that we're writing tests in the same way! I initially enabled the rule for the entire monorepo, but since this caused many errors and I wasn't sure whether the rule should apply to non-e2e-test code, I ended up setting the rule as an override only for the Portals e2e tests directory. Happy to create a ticket if we'd like to enable the rule for the entire monorepo though!

I think it would be good to create the ticket since we can use the void keyword to be explicit about the places where we don't want to handle the promise. Do you mind creating it?

Sure! https://sagebionetworks.jira.com/browse/PORTALS-2894

hallieswan · 2024-01-06T00:32:02Z

.github/workflows/build-test-e2e.yml

@@ -85,6 +99,12 @@ jobs:
        run: yarn install --frozen-lockfile
      - name: Install Playwright Browsers
        run: yarn playwright install --with-deps
+      - name: Confirm Docker container is running


Trying to troubleshoot the TypeError: The argument 'file' cannot be empty. Received '' error, as seen in this run. I can reproduce the error when the SWC war does not exist or when the docker container is not running (likely because in CI, we pass a stub to the PW webServer command).

hallieswan · 2024-01-06T00:37:24Z

e2e/discussions.spec.ts

-  page: Page,
-  threadTitle: string,
-) => {
+const getDiscussionParentActionButtons = (page: Page, threadTitle: string) => {


Async arrow function 'getDiscussionParentActionButtons' has no 'await' expression

hallieswan · 2024-01-06T00:37:54Z

e2e/discussions.spec.ts

-let fileHandleIds: string[] = []
+const fileHandleIds: string[] = []


fileHandleIds is never reassigned - prefer-const

hallieswan · 2024-01-06T00:42:43Z

e2e/files.spec.ts

-  testAuth(
-    'should create and delete a file',
-    async ({ userPage, browserName }) => {


I removed browserName, since it wasn't used. This resulted in many whitespace changes, so the diff isn't very helpful -- no other changes were made in this file

Thanks, this was a catastrophic diff

hallieswan · 2024-01-06T00:45:55Z

e2e/fixtures/authenticatedUserPages.ts

+    // eslint-disable-next-line  @typescript-eslint/no-unused-vars
    async ({ browser, browserName, createUsers }, use) => {


createUsers is not used, but must be included so that the createUser worker fixture creates each test user before this worker fixture runs. Ignore this type error.

It's not intuitive to me why createUsers needs to be destructured in the signature to accomplish different behavior. Could you explain or point me to some documentation? And also leave a comment alongside the comment used to disable the rule?

Sure! The destructured fixtures are the dependencies of the current fixture, as described in the Execution Order docs. Since the test users must be created before the test user can be authenticated, the storageStatePaths worker fixture depends on the createUsers worker fixture. This tells Playwright to set up createUsers before executing storageStatePaths. I'll add a comment to clarify in the code!

hallieswan · 2024-01-06T00:46:33Z

e2e/helpers/ACCESS_TYPE.ts

Import ACCESS_TYPE from @sage-bionetworks/synapse-types instead

hallieswan · 2024-01-06T00:48:41Z

e2e/helpers/acl.ts

+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
+      return (await SRC.SynapseClient.getEntityACL(
+        entityId,
+        accessToken,
+      )) as AccessControlList


Defining an object in SWC that represents SRC would be difficult, so ignore these errors for now since we plan to clean this up once SRC can be called directly from the e2e tests.

This is still primarily blocked by ESM/CJS mismatch issues, correct?

Yes, importing SRC directly is blocked by ESM/CJS issues. However, we'd also considered moving SynapseClient into its own package that could run in browser or node.js environments, so that we wouldn't have to resolve the ESM/CJS issues for all of SRC. I think this work was blocked by the OpenAPI work.

hallieswan · 2024-01-06T00:51:29Z

e2e/helpers/tables.ts


  // skip initial empty column
  await expectTableColumnHeaders(tableRows, columnsSchemaConfig, 1)

  await test.step('enter test data', async () => {
    const addRowButton = page.getByRole('button', { name: 'Add Row' })
    for (let index = 0; index < tableData.length; index++) {
-      await test.step(`enter test data: ${tableData[index].item}`, async () => {
+      const item = tableData[index].item.toString()


item can be a string, string[], or number, so convert to a string before using in the template expression

hallieswan · 2024-01-06T00:52:24Z

e2e/helpers/tables.ts

-        const expectedText =
-          await test.step('format expected text', async () => {


Removed the unnecessary async, which resulted in whitespace changes. No other changes in this chunk

hallieswan · 2024-01-06T00:53:16Z

e2e/helpers/types.ts

@@ -1,12 +1,4 @@
-import ACCESS_TYPE from './ACCESS_TYPE'


Types are imported from @sage-bionetworks/synapse-types, rather than duplicated here

hallieswan · 2024-01-06T00:57:26Z

tsconfig.json

@@ -0,0 +1,31 @@
+{


Other than the files/paths in "files" and "include", this is almost an exact copy of tsconfig.base.json in synapse-react-client. I'm not very familiar with TypeScript compiler options, so if there are more appropriate options, happy to adjust this as needed.

nickgros

This is a great improvement, just had one question/request to add a comment in code.

nickgros · 2024-01-08T14:12:01Z

.eslintrc.js

+    '@typescript-eslint/no-inferrable-types': 'off',
+    '@typescript-eslint/ban-ts-comment': 'warn',
+    '@typescript-eslint/no-empty-interface': 'warn',
+    '@typescript-eslint/no-floating-promises': 'error',


Do you think we should create a ticket to enable this rule in synapse-web-monorepo? I think it makes sense since we're writing tests in essentially the same way

nickgros · 2024-01-08T14:17:35Z

e2e/files.spec.ts

-  testAuth(
-    'should create and delete a file',
-    async ({ userPage, browserName }) => {


Thanks, this was a catastrophic diff

nickgros · 2024-01-08T14:20:25Z

e2e/fixtures/authenticatedUserPages.ts

+    // eslint-disable-next-line  @typescript-eslint/no-unused-vars
    async ({ browser, browserName, createUsers }, use) => {


It's not intuitive to me why createUsers needs to be destructured in the signature to accomplish different behavior. Could you explain or point me to some documentation? And also leave a comment alongside the comment used to disable the rule?

nickgros · 2024-01-08T14:23:03Z

e2e/helpers/acl.ts

+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
+      return (await SRC.SynapseClient.getEntityACL(
+        entityId,
+        accessToken,
+      )) as AccessControlList


This is still primarily blocked by ESM/CJS mismatch issues, correct?

hallieswan added 3 commits January 5, 2024 11:29

SWC-6621: lint e2e tests, use types from @sage-bionetworks/synapse-types

441aa61

SWC-6621: confirm docker container is running before running e2e tests

91c81a0

SWC-6621: use messages types from @sage-bionetworks/synapse-types

c55db09

hallieswan commented Jan 6, 2024

View reviewed changes

hallieswan marked this pull request as ready for review January 6, 2024 00:58

hallieswan requested a review from nickgros January 6, 2024 00:58

nickgros approved these changes Jan 8, 2024

View reviewed changes

hallieswan force-pushed the SWC-6621 branch 12 times, most recently from f048ca8 to a1031d9 Compare January 9, 2024 22:49

SWC-6621: improve documentation per PR

9de76bc

hallieswan force-pushed the SWC-6621 branch from a1031d9 to 9de76bc Compare January 10, 2024 19:21

hallieswan merged commit 7670df9 into Sage-Bionetworks:develop Jan 10, 2024
2 checks passed

hallieswan deleted the SWC-6621 branch January 10, 2024 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SWC-6621 #5250

SWC-6621 #5250

hallieswan commented Jan 6, 2024

hallieswan Jan 6, 2024

nickgros Jan 8, 2024

hallieswan Jan 8, 2024

nickgros Jan 10, 2024

hallieswan Jan 10, 2024

hallieswan Jan 6, 2024

hallieswan Jan 6, 2024

hallieswan Jan 6, 2024

hallieswan Jan 6, 2024

nickgros Jan 8, 2024

hallieswan Jan 6, 2024

nickgros Jan 8, 2024

hallieswan Jan 8, 2024

hallieswan Jan 6, 2024 •

edited

Loading

hallieswan Jan 6, 2024

nickgros Jan 8, 2024

hallieswan Jan 8, 2024

hallieswan Jan 6, 2024

hallieswan Jan 6, 2024

hallieswan Jan 6, 2024

hallieswan Jan 6, 2024

nickgros left a comment

nickgros Jan 8, 2024

nickgros Jan 8, 2024

nickgros Jan 8, 2024

nickgros Jan 8, 2024

		let fileHandleIds: string[] = []
		const fileHandleIds: string[] = []

		// eslint-disable-next-line @typescript-eslint/no-unused-vars
		async ({ browser, browserName, createUsers }, use) => {

		const expectedText =
		await test.step('format expected text', async () => {

SWC-6621 #5250

SWC-6621 #5250

Conversation

hallieswan commented Jan 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallieswan Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallieswan Jan 6, 2024 •

edited

Loading