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

Use and export parameterizeNamedList in nimble-angular #1735

Closed
wants to merge 8 commits into from

Conversation

mollykreis
Copy link
Contributor

Pull Request

🀨 Rationale

Resolves #1551

πŸ‘©β€πŸ’» Implementation

There were a few changes made here:

  • Move parameterizeNamedList in nimble-components from utilities/tests to testing to move it from a private directory to a directory that is intended to be public
  • Create a new testing entry point in the nimble-angular package that re-exports the parameterize utilities
  • Update Angular tests that used forEach to loop through test cases to use parameterizeNamedList instead

πŸ§ͺ Testing

Ran the tests and verified the same number of tests are still being run

βœ… Checklist

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

@mollykreis
Copy link
Contributor Author

@m-akinc, will you buddy this for me?

@mollykreis mollykreis requested a review from m-akinc January 5, 2024 16:53
@mollykreis mollykreis marked this pull request as ready for review January 5, 2024 17:36
@@ -1,4 +1,4 @@
import { parameterize, parameterizeNamedList } from '../parameterized';
import { parameterize, parameterizeNamedList } from './parameterized';
Copy link
Member

Choose a reason for hiding this comment

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

The spec file should be in a tests folder so it's omitted from the build
/src/testing/tests/

@@ -3,6 +3,7 @@ import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testin
import { Router } from '@angular/router';
Copy link
Member

Choose a reason for hiding this comment

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

Discussed how the parameterize test helper could be useful in skyline as well and is maybe not good to expose via nimble-components or nimble-angular packages (it's a generic jasmine helper not tied to our components).

Directions discussed:

  • Create a separate package in nimble, like @ni/jasmine-parameterized (which is similar to this public package except ours would be in the ni scope)
    • Would need typescript compile and jasmine test set-up
    • Would actually be neat to run it in node and and in browser, should be generic enough to, the typescript build would need to emit dist/esm and dist/cjs
  • Copy and paste the utility to the @ni/nimble-angular/internal-utilities entry point so it isn't part of the publically supported api and make the package later in nimble or the js style guide.

Copy link
Member

Choose a reason for hiding this comment

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

Moving to draft temporarily until a direction is picked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #1738, I am creating a new @ni/jasmine-parameterized package with these utilities. Once that PR is merged, I plan to update this PR to reference the new location of parameterizeNamedList.

@rajsite rajsite marked this pull request as draft January 5, 2024 20:08
mollykreis added a commit that referenced this pull request 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]>
@mollykreis
Copy link
Contributor Author

I'm closing this PR in favor of #1768

@mollykreis mollykreis closed this Jan 23, 2024
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.

Migrate to the parameterize and parameterizeNamedList test helpers
4 participants