Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

fix(loadModule): missing getTenantRootModule #152

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

JAdshead
Copy link
Contributor

Description

Motivation and Context

Is possible that root module is built without getTenantRootModule.
This leads to a bug where child module requires externals, root module does not provide but its skipped validation.

How Has This Been Tested?

Test suite and locally building changes into one-app.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • My changes are in sync with the code style of this project.
  • There aren't any other open Pull Requests for the same issue/update.
  • These changes should be applied to a maintenance branch.
  • This change requires cross browser checks.
  • This change impacts caching for client browsers.
  • This change adds additional environment variable requirements for Holocron users.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using Holocron?

Copy link
Contributor

@PixnBits PixnBits left a comment

Choose a reason for hiding this comment

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

I might need to open a PR to add some expect.assertion calls to the other async tests (out of scope for this PR)
I wonder if we could use an ESLint rule for that? 🤔

@@ -1254,14 +1261,53 @@ describe('loadModule.node', () => {
});
});

it('does not load child module when root module does not set getTenantRootModule', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('does not load child module when root module does not set getTenantRootModule', async () => {
it('does not load child module when root module does not set getTenantRootModule', async () => {
expect.assertions(1);

@JAdshead
Copy link
Contributor Author

I might need to open a PR to add some expect.assertion calls to the other async tests I wonder if we could use an ESLint rule for that? 🤔

Why, it does not provide value for async tests. all it does is clog up the failures

@PixnBits PixnBits requested a review from a team September 18, 2023 18:46
@PixnBits
Copy link
Contributor

Why, [expect.assertions] does not provide value for async tests. all it does is clog up the failures

I've seen test suites pass but upon inspection they were not returning or awaiting the assertions which, when adjusted, were failing.

FWIW looks like this is already a rule:
https://github.com/jest-community/eslint-plugin-jest/blob/main/docs/rules/prefer-expect-assertions.md

@JAdshead
Copy link
Contributor Author

Why, [expect.assertions] does not provide value for async tests. all it does is clog up the failures

I've seen test suites pass but upon inspection they were not returning or awaiting the assertions which, when adjusted, were failing.

FWIW looks like this is already a rule: https://github.com/jest-community/eslint-plugin-jest/blob/main/docs/rules/prefer-expect-assertions.md

this is using await, can you show how can the expect not be called without rewriting the tests ?

Copy link
Contributor

@PixnBits PixnBits left a comment

Choose a reason for hiding this comment

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

enabling jest/prefer-expect-assertions is a separate scope, so different PR

@PixnBits PixnBits merged commit 55daa5d into main Sep 18, 2023
@PixnBits PixnBits deleted the fix/missing-getTenantRootModule branch September 18, 2023 20:27
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants