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

Migrate to the parameterize and parameterizeNamedList test helpers #1551

Closed
rajsite opened this issue Sep 19, 2023 · 0 comments · Fixed by #1768
Closed

Migrate to the parameterize and parameterizeNamedList test helpers #1551

rajsite opened this issue Sep 19, 2023 · 0 comments · Fixed by #1768
Assignees

Comments

@rajsite
Copy link
Member

rajsite commented Sep 19, 2023

🧹 Tech Debt

The PR #1379 introduced new patterns for writing parameterized tests which addresses a bunch of short comings with the existing patterns and deprecated the older getSpecTypeByNamedList.

This tech debt task should:

  • Migrate all existing usages of getSpecTypeByNamedList to parameterizeNamedList
  • Migrate tests in such a way as they take advantage of the strict typing (using as const on the test cases) and consistent names for the test function parameters (spec, name, and value)
  • Remove the deprecated getSpecTypeByNamedList helpers

We also have parameterized tests in the nimble-angular package. These tests generally just are written with forEach (as opposed to the deprecated getSpecTypeByNamedList, which is internal to nimble-components). We should also look at making parameterize/parameterizeNamedList exported from nimble-components so it can be used for our Angular tests too.

@rajsite rajsite added tech debt triage New issue that needs to be reviewed labels Sep 19, 2023
rajsite added a commit that referenced this issue Sep 19, 2023
# Pull Request

## 🤨 Rationale

Updates the parametrized test pattern to address a few shortcomings in
the previous pattern:
- Avoids a lot of duplicated boilerplate
- Doesn't encourage an approach that results in eslint disables being
used often (false positive of await used in a loop)
- Prevents accidentally focusing a test and checking it in
- Gives decent typing, may take run-time to see if it needs improvements

## 👩‍💻 Implementation

Exposed new parameterization helpers named `parameterize` and
`parameterizeNamedList` and marked the existing method deprecated.
Created a tech debt task to track migrating to the new pattern:
#1551

## 🧪 Testing

Added some tests for the new helpers and migrated a couple of existing
tests to the new pattern.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed. Added detailed comments to the helper
like the deprecated helper did.
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Sep 19, 2023
@mollykreis mollykreis self-assigned this Jan 2, 2024
rajsite added a commit that referenced this issue Jan 4, 2024
# Pull Request

## 🤨 Rationale

This PR addresses the first part of #1551. It migrates tests in
`nimble-components` to the `parameterizeNamedList` utility and deletes
the deprecated `getSpecTypeByNamedList`.

To complete #1551, `parameterizeNamedList` needs to be exported and used
from `nimble-angular`, but this will be tackled in a separate PR.

## 👩‍💻 Implementation

- Use `parameterizeNamedList` in the places where
`getSpecTypeByNamedList` had been used (plus a few more places that
weren't using either)
- Delete `getSpecTypeByNamedList`

## 🧪 Testing

Ran unit tests

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Milan Raj <[email protected]>
@m-akinc m-akinc moved this from Backlog to In progress in Nimble Design System Priorities Jan 8, 2024
mollykreis added a commit that referenced this issue Jan 23, 2024
# Pull Request

## 🤨 Rationale

This is part of #1551

Based on [this
discussion](#1735 (comment)),
we decided to create a new npm package for `jasmine-parameterized`
rather than have it be exported from an existing nimble package.

## 👩‍💻 Implementation

- Create a new package called `jasmine-parameterized` that contains the
existing `parameterized.ts` and `tests\parameterized.spec.ts`
- Add appropriate support files to build, lint, and test the package
- Delete `parameterized.ts` and `tests\parameterized.spec.ts` from
`nimble-components`
- Update the tests in `nimble-components` to use `parameterizeSpec`
from `jasmine-parameterized`

## 🧪 Testing

- Verified that the jasmine-parameterized tests are run in the pipeline
- Verified that the jasmine-parameterized package contains the `dist`
directory (without tests), `package.json`, and `README.md`
- Verified all nimble-components tests still pass

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Jesse Attas <[email protected]>
Co-authored-by: rajsite <[email protected]>
rajsite added a commit that referenced this issue Jan 24, 2024
# Pull Request

## 🤨 Rationale

Resolves #1551 

The last step of #1551 is to use the new `@ni/jasmine-parameterized`
package in the Angular tests.

## 👩‍💻 Implementation

Replace usages of `testCases.forEach` in tests to use `parameterizeSpec`
from `@ni/jasmine-parameterize` instead.

## 🧪 Testing

- Verified the same number of tests still run & pass

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Milan Raj <[email protected]>
@m-akinc m-akinc moved this from In progress to Done in Nimble Design System Priorities Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants