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

Jest test has many #97

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Jest test has many #97

merged 1 commit into from
Oct 22, 2021

Conversation

dulerong
Copy link
Collaborator

@dulerong dulerong commented Oct 19, 2021

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe): test

What is the current behaviour?

Issue Number(s): N/A

What is the new behaviour?

  • add tests for has_many.js

Other information

N/A

@dulerong
Copy link
Collaborator Author

@ilunglee
Hey, this time I squashed all my commits before pushing, could you please take a look and let me know what you think?

Thanks.

Copy link
Contributor

@ilunglee ilunglee left a comment

Choose a reason for hiding this comment

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

Hey @dulerong, thanks for the great work! Please see my comments.

Commit message

When squashing your commits, please ensure you clean up the commit message. For example, I recommend changing the commit message for 9bfc4c8 to:

commit 9bfc4c8e8ea7b54cc79a3c5c2d208fcc24e98d8f
Author: dulerong <codeydu@hotmail.com>
Date:   Tue Oct 12 12:22:48 2021 +0900
    test: add tests for has_many.js

Please see CONTRIBUTING.md for more information.

HTML templates for Jest testings

I'm wondering if it would make sense for us to use jsdom and load an example page (i.e., has_many.html) for jest testings.

document.body.innerHTML = fs.readFileSync("./fixtures/has_many.html");

It will be easier to maintain and much clearer to read this way, and we don't need to create the elements manually. See the following links for more information:


__tests__ folder structure

Also, not related to this PR, I'm wondering if we should move the __tests__ folder from app/javascript/adminterface/lib/__tests__ to app/javascript/adminterface/__tests__ folder to represent all the JavaScript files and move the files to their respective folder paths (e.g., __tests__/has_many.spec.js -> __tests__/lib/has_many.spec.js).

What do you think? Thanks!

app/javascript/adminterface/lib/__tests__/has_many.spec.js Outdated Show resolved Hide resolved
test: has_many -> recomputePosition

chore: change web api

test: bindDestroyEvent

test: bindRemoveEvent

test: bindAddEvent

chore: refactored DOM manipulation, deleted useless code

test: bindRemoveEventCallBack

chore: apply new DOM node to tests

test: bindAddEventCallBack

refactor: event call back function

chore: change mock to spy

chore: change mock to spy

chore: lint

test: initSortable new assert

test: bindAddEventCallBack more assertions

chore: delete unnecessary code

refactor: move html elements to separate file

refactor: add html file for test
@ilunglee ilunglee merged commit 5d0742c into main Oct 22, 2021
@ilunglee ilunglee deleted the jest_test_has_many branch October 22, 2021 01:25
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