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

Create jasmine-parameterized package #1738

Merged
merged 21 commits into from
Jan 23, 2024
Merged

Create jasmine-parameterized package #1738

merged 21 commits into from
Jan 23, 2024

Conversation

mollykreis
Copy link
Contributor

@mollykreis mollykreis commented Jan 8, 2024

Pull Request

🤨 Rationale

This is part of #1551

Based on this discussion, 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

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

@mollykreis
Copy link
Contributor Author

@msmithNI, can you buddy this PR for me?

@mollykreis mollykreis requested a review from msmithNI January 8, 2024 18:09
packages/jasmine-parameterized/README.md Outdated Show resolved Hide resolved
@mollykreis mollykreis marked this pull request as ready for review January 9, 2024 20:00
mollykreis and others added 2 commits January 12, 2024 13:55
Co-authored-by: Jesse Attas <jattasNI@users.noreply.github.com>
@@ -1,6 +1,7 @@
/* eslint-disable max-classes-per-file */
import { customElement, html, ref } from '@microsoft/fast-element';
import { MenuItem as FoundationMenuItem } from '@microsoft/fast-foundation';
import { parameterizeNamedList } from '@ni/jasmine-parameterized/dist/esm/parameterized';
Copy link
Member

@rajsite rajsite Jan 16, 2024

Choose a reason for hiding this comment

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

Thinking about the API a bit, I think we should make the following changes:

It shouldn't be in this PR but this allows us to make something like parameterizeSuite to align with parameterizeSpec in the future, something like:

const sizes = [
    { name: 'small', value: 2 },
    { name: 'medium', value: 5 },
    { name: 'large', value: 10}
] as const;
parameterizeSuite(sizes, (suite, suiteName, suiteValue) => {
  suite(`A ${suiteName} rain`, () => {
     const rainTests = [
          { name: 'cats-and-dogs', type: 'idiom' },
          { name: 'frogs' type: 'idiom'},
          { name: 'men', type: 'lyrics'}
      ] as const;
      parameterizeSpec(rainTests, (spec, name, value) => {
          spec(`of type ${name} exist`, () => {
              expect(value.type).toBeDefined();
              expect(suiteValue.value).toBeDefined();
          });
      }, {
          'cats-and-dogs': fit,
          frogs: xit
      });
  });
}, {
  small: xdescribe
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't expose the parameterize helper function (i.e. don't export it or put it in the readme) and rename parameterizeNamedList to parameterizeSpec

I've renamed parameterizeNamedList to parameterizeSpec. I also no longer am exporting parameterize or including it in the readme. However, my question is: Should we remove parameterize altogether? It isn't used by anything.**

Make it so the import is top-level in the package, i.e. import { parameterizeSpec } from '@ni/jasmine-parameterized';. This should be doable with conditional exports for import and require: https://nodejs.org/api/packages.html#conditional-exports

Done, but let me know if there is something I should've done differently.

We create the cjs build and we should test it in node

Done. I also updated karma.conf.json to only run tests on the esm build, but should karma.conf.json still be configured to run for both esm and commonjs?

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove parameterize altogether? It isn't used by anything.**

It is used in the parameterizeSpec implementation. Don't think I follow what is being proposed.

parameterize<ObjectFromNamedList<T>>(testCases, test, specOverrides);

Done. I also updated karma.conf.json to only run tests on the esm build, but should karma.conf.json still be configured to run for both esm and commonjs?

Was mulling over it a bit and realized that the browser usage was being pushed through webpack, so it was really testing the webpack config and the node usage wasn't relying on the cjs import as the test file itself was using the es6 import statement to reference the dependency instead of the cjs format require statement.

Modern versions of browsers and node support es6 modules so I simplified and removed the cjs parts of the build. I switched the TS config to use the newer "Node16" for the module format (which is just es6 modules as supported by node).

So now we are actually using esm js files and I got esm tests to work in karma .

I also read the npm entrypoints and typescript npm entrypoint docs and modified the format of the entrypoints to match that. For some reason I needed the Fall-back for older versions of TypeScript that I couldn't figure out but guess it's not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the updates to the branch!

@mollykreis mollykreis requested a review from rajsite January 18, 2024 22:14
@rajsite rajsite changed the title Create jasmine-parameterized workspace Create jasmine-parameterized package Jan 23, 2024
@mollykreis mollykreis merged commit 609265d into main Jan 23, 2024
11 checks passed
@mollykreis mollykreis deleted the jasmine-parameterized branch January 23, 2024 15:43
@rajsite rajsite mentioned this pull request Feb 14, 2024
1 task
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.

4 participants