chore: add-tests by ryanbas21 · Pull Request #15 · ForgeRock/ping-javascript-sdk

ryanbas21 · 2025-02-11T23:40:03Z

JIRA Ticket

https://pingidentity.atlassian.net/browse/SDKS-3677

Description

Adds some testing, added a test to the "basic" suite because i wanted to make sure logout worked.

This is probably dependent on a WIP story because the fields flow uses things like password verify and label which im purposefully asserting as undefined.

When that work is done and this gets rebased, i'd expect these tests to fail

changeset-bot · 2025-02-11T23:40:07Z

🦋 Changeset detected

Latest commit: 194133a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@forgerock/davinci-client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

nx-cloud · 2025-02-11T23:40:41Z

View your CI Pipeline Execution ↗ for commit 194133a.

Command	Status	Duration	Result
`nx affected -t build typecheck lint test e2e-ci`	✅ Succeeded	1m 4s	View ↗
`nx-cloud record -- nx format:check`	✅ Succeeded	3s	View ↗

☁️ Nx Cloud last updated this comment at 2025-03-06 10:25:46 UTC

github-actions · 2025-02-11T23:42:50Z

Deployed eb19543 to https://ForgeRock.github.io/ping-javascript-sdk/pr-15/eb195434f2a94373d5644417ee57680979364276 branch gh-pages in ForgeRock/ping-javascript-sdk

ryanbas21 · 2025-02-18T17:09:04Z

  CollectorType extends SingleValueCollectorTypes = 'SingleValueCollector',
 >(field: Field, idx: number, collectorType: CollectorType, data?: string) {
  let error = '';
+  console.log('the field ', field);


debugging because label was resolving to an error during normalization.

codecov-commenter · 2025-02-18T17:12:33Z

Codecov Report

Attention: Patch coverage is 50.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 50.23%. Comparing base (c2ba0e7) to head (194133a).

Files with missing lines	Patch %	Lines
packages/davinci-client/src/lib/client.store.ts	0.00%	19 Missing ⚠️
packages/davinci-client/src/lib/davinci.utils.ts	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   50.00%   50.23%   +0.23%     
==========================================
  Files          21       21              
  Lines        1232     1256      +24     
  Branches      163      166       +3     
==========================================
+ Hits          616      631      +15     
- Misses        616      625       +9

Files with missing lines	Coverage Δ
packages/davinci-client/src/lib/node.reducer.ts	`76.92% <100.00%> (+0.78%)`	⬆️
packages/davinci-client/src/lib/davinci.utils.ts	`72.61% <89.47%> (+1.28%)`	⬆️
packages/davinci-client/src/lib/client.store.ts	`0.00% <0.00%> (ø)`

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ryanbas21 · 2025-02-18T21:16:17Z

@@ -1,6 +1,5 @@
 name: ForgeRock Pull Request CI
 on:
-  pull_request_target:


this was a mistake to add. sorry. its not what i thought it would do.

ryanbas21 · 2025-02-18T21:16:35Z

-const config: DaVinciConfig = {
-  clientId: '724ec718-c41c-4d51-98b0-84a583f450f9',
-  redirectUri: window.location.origin + '/',
+const qs = window.location.search;


added logic to add clientId param to tests for variability of flows.

ryanbas21 · 2025-02-18T21:16:56Z

    const loginBtn = document.getElementById('logoutButton') as HTMLButtonElement;
    loginBtn.addEventListener('click', async () => {
-      await FRUser.logout({ logoutRedirectUri: window.location.href });
+      await FRUser.logout({ logoutRedirectUri: `${window.location.origin}/` });


because of the above changes , fixed this to use origin so it doesnt include the clientId param

ryanbas21 · 2025-02-18T21:17:15Z

+      return true;
+    }
+  });
+  await logoutButton.click();


extended this test to include logout for my own sanity.

ryanbas21 · 2025-02-18T23:11:19Z

this is why i'm adding a changeset.

cerebrl

Got some questions and some suggestions. The biggest concern I have is the test suite code that seems like to have quite a bit of redundancy that doesn't provide any value that I can see.

cerebrl · 2025-02-24T17:25:19Z

+  await expect(page.getByRole('link', { name: 'Vite logo' })).toBeVisible();
+  await expect(page.getByRole('button', { name: 'Form Validation' })).toBeVisible();
+  await expect(page.locator('#form')).toContainText('Form Validation');
+  await page.getByRole('button', { name: 'Form Validation' }).click();


Why is there a button called "Form Validation"?

the way the connector is it renders two buttons, one is form validation and one is form fields and then we render those fields out.

cerebrl · 2025-02-24T17:27:08Z

+  await page.getByRole('textbox', { name: 'Username' }).click();
+  await page.getByRole('textbox', { name: 'Username' }).fill('sdk-user');


I feel like the clicking is not really doing anything, since your reselecting the element to .fill it. We probably should just remove these unneeded .click calls.

cerebrl · 2025-02-24T17:28:02Z

+  await expect(page.getByText('Username')).toBeVisible();
+  await expect(page.getByRole('textbox', { name: 'Username' })).toBeVisible();


Couldn't these two lines be collapsed into a .getByLabel selector?

cerebrl · 2025-02-24T17:30:03Z

+  await page.getByRole('textbox', { name: 'Username' }).fill('sdk-user');
+  await expect(page.getByRole('textbox', { name: 'Username' })).toHaveValue('sdk-user');
+  await page.getByRole('textbox', { name: 'Email Address' }).click();
+  await page.getByRole('textbox', { name: 'Email Address' }).fill('sdk-user@user.com');


I feel like this is quite redundant. I personally feel this could be reduced to a page.getByLabel('Username').fill('demouser');. That would, how I understand it, reproduce the same result with no loss of testing.

cerebrl · 2025-02-24T17:35:33Z

+  await page.getByRole('heading', { name: 'Select Test Form' }).click();
+  await page.getByRole('button', { name: 'Form Fields' }).click();
+  await page.getByRole('textbox', { name: 'Text Input Label' }).click();
+  await page.getByRole('textbox', { name: 'Text Input Label' }).click();
+
+  const txtInput = page.getByRole('textbox', { name: 'Text Input Label' });
+  await txtInput.fill('This is some text');
+  expect(txtInput).toHaveValue('This is some text');
+
+  const flowLink = page.getByRole('button', { name: 'Flow Link' });
+  await flowLink.click();
+
+  const flowButton = page.getByRole('button', { name: 'Flow Button' });
+  await flowButton.click();
+
+  const requestPromise = page.waitForRequest((request) => request.url().includes('/customForm'));
+  await page.getByRole('button', { name: 'Submit' }).click();
+  const request = await requestPromise;
+  const parsedData = JSON.parse(request.postData());
+  const data = parsedData.parameters.data;
+  expect(data.actionKey).toBe('submit');
+  expect(data.formData).toEqual({
+    // leaving this here because it should be fixed and we would have a failing test when we do fix import {  } from "// to remind us to update the test"
+    ['undefined']: '',
+    ['text-input-key']: 'This is some text',
+    ['dropdown-field-key']: '',
+    ['radio-group-key']: '',
+  });
+});


I can't figure out what this is actually doing/testing. Can you elaborate?

well we are going to test that when we click to send off the customForm request, we are sending the data correctly in the actual request.

cerebrl · 2025-02-24T17:37:02Z

+  expect(data.formData).toEqual({
+    ['undefined']: '',
+    'user.username': '',
+    'user.password': '',
+    'user.email': '',
+  });


Why are we expecting these values to be empty strings?

Because i was leaving this here so the test would fail once your PR was merged. which happened

cerebrl · 2025-02-24T19:27:56Z

+    const hasNextUrl = () => {
+      const data = cacheEntry.data;
+
+      if ('_links' in data) {
+        if ('next' in data._links) {
+          if ('href' in data._links.next) {
+            return true;
+          }
+        }
+      }
+      return false;
+    };


I feel like this should be a pure, stateless utility that could be used for fetching any property within _links. This could be reused in other portions of the project.

cerebrl · 2025-03-05T16:15:36Z


      return function (value: string, index?: number) {
        try {
+          console.log('the value', value);


We should probably remove this.

cerebrl · 2025-03-05T16:24:22Z

+      if (
+        collectorToUpdate.category !== 'SingleValueCollector' &&
+        collectorToUpdate.category !== 'ValidatedSingleValueCollector'
+      ) {


Did I miss checking for MultiValueCollectors as well?

form-field validations were not being sent in requests.

ryanbas21 added the do not merge label Feb 11, 2025

ryanbas21 force-pushed the support-field-tests branch from 6012e48 to b1e4f40 Compare February 18, 2025 17:08

ryanbas21 commented Feb 18, 2025

View reviewed changes

ryanbas21 force-pushed the support-field-tests branch from cae1356 to 72b3ce5 Compare February 18, 2025 21:09

ryanbas21 commented Feb 18, 2025

View reviewed changes

ryanbas21 force-pushed the support-field-tests branch 3 times, most recently from 9424cab to e89b4e4 Compare February 18, 2025 23:06

ryanbas21 commented Feb 18, 2025

View reviewed changes

Comment thread packages/davinci-client/src/lib/davinci.utils.ts

Copy link
Copy Markdown

Collaborator Author

ryanbas21 Feb 18, 2025

Choose a reason for hiding this comment

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

this is why i'm adding a changeset.

cerebrl requested changes Feb 24, 2025

View reviewed changes

ryanbas21 force-pushed the support-field-tests branch from 7857a22 to d495187 Compare February 25, 2025 22:26

ryanbas21 force-pushed the support-field-tests branch from d495187 to d430d9e Compare March 5, 2025 12:32

cerebrl requested changes Mar 5, 2025

View reviewed changes

ryanbas21 force-pushed the support-field-tests branch from d430d9e to c3d1da1 Compare March 5, 2025 16:45

ryanbas21 removed the do not merge label Mar 5, 2025

ryanbas21 added 5 commits March 6, 2025 03:52

chore: add-tests

3b3c554

chore: fix-logic-add-tests

1e24446

chore: add-logout-to-test

9569ae5

chore: add-changeset

a8c7124

chore: fixes-for-form-fields

6fa225e

form-field validations were not being sent in requests.

cerebrl force-pushed the support-field-tests branch from c3d1da1 to 33d3bb3 Compare March 6, 2025 10:16

cerebrl approved these changes Mar 6, 2025

View reviewed changes

feat(davinci-client): bug fixes, improvements for MultiValueCollectors

194133a

cerebrl force-pushed the support-field-tests branch from 33d3bb3 to 194133a Compare March 6, 2025 10:23

cerebrl merged commit 9d96a9b into main Mar 6, 2025

cerebrl deleted the support-field-tests branch March 6, 2025 10:28

ryanbas21 mentioned this pull request Mar 6, 2025

Release PR #11

Merged

		await page.getByRole('textbox', { name: 'Username' }).click();
		await page.getByRole('textbox', { name: 'Username' }).fill('sdk-user');

		await expect(page.getByText('Username')).toBeVisible();
		await expect(page.getByRole('textbox', { name: 'Username' })).toBeVisible();

Conversation

ryanbas21 commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA Ticket

Description

Uh oh!

changeset-bot Bot commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

nx-cloud Bot commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

ryanbas21 commented Feb 11, 2025 •

edited

Loading

changeset-bot Bot commented Feb 11, 2025 •

edited

Loading

nx-cloud Bot commented Feb 11, 2025 •

edited

Loading

github-actions Bot commented Feb 11, 2025 •

edited

Loading

codecov-commenter commented Feb 18, 2025 •

edited

Loading