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

chore: upgrade jasmine-core and karma-jasmine #3890

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

nolanlawson
Copy link
Collaborator

@nolanlawson nolanlawson commented Dec 6, 2023

Details

We were previously blocked from upgrading Jasmine because of lack of IE11 support, as well as a breaking change that broke our tests: jasmine/jasmine#1742

The breaking change in Jasmine is basically that Jasmine complains due to the "empty" describe():

describe('foo', () => {
  if (SOME_ENV_CONDITION_THAT_IS_FALSE) {
    it(...)
  }
})

Of course we do this a lot.

IE11 is no longer a problem, and we can make some small tweaks to our tests to support the breaking change in Jasmine. This allows us to upgrade.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

@nolanlawson nolanlawson requested a review from a team as a code owner December 6, 2023 23:36
Copy link
Contributor

❌ An unexpected error occurred while attempting to start the test stage of workflow 191737. Please try to start the stage again, or reach out to #nucleus-talk for help.

.join('\n');
const intro =
ancestorDirectories.map((tag) => `describe("${tag}", function () {`).join('\n') +
`\nxit("dummy test", () => { /* empty */ });\n`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not super happy with this, but there's no config option to change Jasmine's default behavior.

};

fn(elm);
});
Copy link
Collaborator Author

@nolanlawson nolanlawson Dec 6, 2023

Choose a reason for hiding this comment

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

Apparently this done was firing multiple times in native lifecycle mode. By using a Promise, it only fires the resolve once and Jasmine stops complaining.

}
});
});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This, and the other fixes below, are just fixing empty describes.

@nolanlawson nolanlawson merged commit 8b6fd77 into master Dec 14, 2023
9 checks passed
@nolanlawson nolanlawson deleted the nolan/upgrade-jasmine branch December 14, 2023 20:55
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.

2 participants