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

changes to issue #1558 regarding failed Jest test cases #1577

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

binkmywink
Copy link

I was able to make all the tests work by rewriting the CSSTransform file and updating the Jest version in package.json (this can also be done by running npm update jest. I used babel npm install babel-jest --save-dev to reconfigure some files, as I believe that was causing some of the tests to fail. The main goal was to configure Jest for ECMAScript Modules, as the issue came from trying to import d3 using ECMAScript Module syntax (import * as d3 from 'd3') in the Histogram test. Using the minified version of d3 resolved this issue. In addition to updating snapshots before running backend tests, I was able to compile all the tests successfully.

@zqian zqian changed the title changes to issue 1558 regarding failed Jest test cases changes to issue #1558 regarding failed Jest test cases Apr 26, 2024
@zqian zqian requested a review from pushyamig April 26, 2024 16:02
@@ -1,4 +1,4 @@
import * as d3 from 'd3'
import * as d3 from 'd3/dist/d3.min'
Copy link
Contributor

@pushyamig pushyamig Apr 26, 2024

Choose a reason for hiding this comment

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

I don't feel we import things from dist folder. This change effects the View. why are you doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

None of the imports in the project we use dist in path while importing. This needs to be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

On my end I believe there is a parsing issue when I leave the import line as import * as d3 from 'd3'

Copy link
Contributor

Choose a reason for hiding this comment

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

As per my understanding we cannot have an import from dist folder. That mean something in the way we process this needs to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, I'll look into what could be changed

["@babel/preset-react",{"runtime": "automatic"}]]
[
"@babel/preset-env",
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to keep this format same as before. What was the reason for re-formatting this?

// // The output is always the same.
// return 'cssTransform';
// }
// };
Copy link
Contributor

Choose a reason for hiding this comment

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

You should always remove commented code , if not used

@zqian zqian linked an issue Apr 26, 2024 that may be closed by this pull request
@pushyamig
Copy link
Contributor

pushyamig commented Apr 26, 2024

Can you email me or share your Uniqname? we are keeping track of students from EECS course who are contributing to our repo

Copy link
Contributor

@pushyamig pushyamig left a comment

Choose a reason for hiding this comment

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

I have commented on code and I felt needs change

@pushyamig
Copy link
Contributor

pushyamig commented Jul 16, 2024

@jxiao21 I suggest if you are assigned with this issue, you either have to take these change into you branch and have initial commit from contributor as below.

  1. Create diff of this PR as https://github.com/tl-its-umich-edu/my-learning-analytics/pull/1577.diff
  2. Save this diff to you local machine
  3. create a local branch and apply this diff git apply 1577.diff
  4. Make a first commit from this user as author git commit --author="John Doe <[email protected]>" -m "Fixing the JEST. (We want to give credit to the user)

let me know if you have any question. You might have to resolve some conflict/rebase since the code needs to in sync latest from master

@jxiao21
Copy link
Contributor

jxiao21 commented Jul 17, 2024

Forgot to comment this yesterday but right now I may start it from scratch and use the PR as a reference since the current PR is outdated and I'm failing some of the tests with the current changes. Would that still be that would be done with a diff of the current PR?

@pushyamig
Copy link
Contributor

Forgot to comment this yesterday but right now I may start it from scratch and use the PR as a reference since the current PR is outdated and I'm failing some of the tests with the current changes. Would that still be that would be done with a diff of the current PR?

However you want to approach is is fine. If your code changes are adding on from this contributor change. Then have a commit with her name. But that could be added once you fixed this issue (you could do something like this git commit --amend --author="Author Name <[email protected]>" --no-edit)

But as of now you can work on fixing the issue and not worry committing with author name. I can help with that.

@jonespm jonespm marked this pull request as draft October 24, 2024 18:12
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.

Fix Jest Frontend tests
3 participants