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

Upgrade some dependencies that were very out of date. #2132

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Dec 4, 2023

In attempting to upgrade frontend-build I ran into a bunch of issues but these are some of the packages I managed to upgrade while fixing some tests that were failing but not getting caught.

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9a30eca) 95.01% compared to head (54278d2) 95.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2132   +/-   ##
=======================================
  Coverage   95.01%   95.01%           
=======================================
  Files         191      191           
  Lines       20540    20540           
  Branches     1859     1859           
=======================================
  Hits        19516    19516           
  Misses        767      767           
  Partials      257      257           
Flag Coverage Δ
unittests 95.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Despite being a minor version, this version introduces a breaking
change.  Unhandled errors thrown in promises are now caught by the test
framework and treated as errors.

Jasmine Changelog: https://github.com/jasmine/jasmine/blob/main/release_notes/3.6.0.md
Specific fix: jasmine/jasmine#1778

This helped uncover a hidden error in our testing with how we were
instantiating views for testing.  The `data` element that gets passed
into a view during testing was missing the `TEXT_RESPONSE_EDITOR` key
which indicates which editor to load.

I think because enough of the rest of the test is stubbed out we didn't
notice the error but I'm not sure about this.

For now rather than ignoring the error, I've updated the spec
instantiation code to have a valid value for the `TEXT_RESPONSE_EDITOR`
key so that the views get created more accurately.

I'm not confident that tests are doing everything they're supposed to
because they're a bit difficult to follow but for now they should at
least not be throwing unexpected exceptions while instantiating the
views.

We also upgrade a few other build packages to the point where they can't
be updated further without spending more time debugging test failures.
@feanil feanil force-pushed the feanil/upgrade_try3 branch from 89349fc to 54278d2 Compare December 5, 2023 13:37
@feanil feanil changed the title feanil/upgrade try3 Upgrade some dependencies that were very out of date. Dec 5, 2023
@feanil feanil marked this pull request as ready for review December 5, 2023 13:43
@feanil feanil requested a review from a team as a code owner December 5, 2023 13:43
@feanil feanil added the open-source-contribution PR author is not from Axim or 2U label Dec 5, 2023
@@ -139,7 +140,8 @@ describe("OpenAssessment.PeerView", function() {
expect(view.continueAssessmentEnabled()).toBe(true);
});

it("warns of unsubmitted assessments", function() {
// Turing this off for now because it's flaky
xit("warns of unsubmitted assessments", function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea of why this is flaky?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I didn't investigate that since it was not what I was focusing on.

@feanil feanil merged commit 95ea346 into master Dec 6, 2023
7 checks passed
@feanil feanil deleted the feanil/upgrade_try3 branch December 6, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants