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 for bold and italic command #543

Merged
merged 8 commits into from
Jul 7, 2020

Conversation

cypherean
Copy link
Contributor

@cypherean cypherean commented Jun 26, 2020

To be merged after #541 . Basic testing of bold and italic function in both modes.

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@cypherean cypherean changed the title Jest test for bold command Jest test for bold and italic command Jun 26, 2020
@Shulammite-Aso
Copy link
Collaborator

This looks great!!

i guess we'll have to add more tests cases as we fix bugs.

@jywarren
Copy link
Member

Wow cool! The test failed -- actually a useful thing to see!

> publiclab-editor@2.0.6 test-ui /home/travis/build/publiclab/PublicLab.Editor
> node ./node_modules/.bin/jest
 FAIL  test/ui-testing/italic.test.js
  Italic Text
    ✕ Adds emphasized text in rich text mode (71ms)
    ✕ Adds emphasized text in markdown mode (112ms)
  ● Italic Text › Adds emphasized text in rich text mode
    TypeError: PL.Util.enableRichTextModeKeyboardShortcut is not a function
      at HTMLDocument.<anonymous> (file:/home/travis/build/publiclab/PublicLab.Editor/dist/PublicLab.Editor.js:21548:11)
      at e (file:/home/travis/build/publiclab/PublicLab.Editor/node_modules/jquery/dist/jquery.min.js:2:30005)
      at t (file:/home/travis/build/publiclab/PublicLab.Editor/node_modules/jquery/dist/jquery.min.js:2:30307)
      at Suite.Object.<anonymous>.describe (test/ui-testing/italic.test.js:9:3)
  ● Italic Text › Adds emphasized text in markdown mode
    expect(received).toBe(expected) // Object.is equality
    Expected: true
    Received: false

https://travis-ci.org/github/publiclab/PublicLab.Editor/jobs/703594757#L293

🎉 🎉

@jywarren
Copy link
Member

Great work getting this started. And thanks a ton for the review, @Shulammite-Aso !!!

@cypherean
Copy link
Contributor Author

@jywarren looked into why the tests were failing and it is because of this #490 (comment) . As indicated here,
Screenshot from 2020-06-30 23-12-22

Also Keshav has fixed this here #490 (comment). Local test results after commenting out PL.Util.enableRichTextModeKeyboardShortcut
Screenshot from 2020-06-30 23-09-25
whereas they failed if we left it as it is. I think the tests should work fine once we get the error fixed in main.

@jywarren
Copy link
Member

OK, let's keep a lookout for that fix and then we can rebase this and mark this ready to merge? Thanks!

@keshav234156
Copy link
Member

keshav234156 commented Jul 2, 2020

ok I will a separate PR to fix and merge this test quickly

…ompletion

added flag to run jest test sequentially
@cypherean
Copy link
Contributor Author

@keshav234156 I've fixed it here itself. Thanks for pointing it out in gsoc planning issue.

@cypherean
Copy link
Contributor Author

I've fixed the previous error and refined the tests as well but the build is failing. Italic and bold tests follow the exact same code, but the italic test works whereas the bold test fails. Everything works fine locally. Any idea what might be wrong?
Screenshot from 2020-07-03 17-21-12

@Shulammite-Aso
Copy link
Collaborator

Maybe you should rebase??

@cypherean
Copy link
Contributor Author

Already done.

@cypherean cypherean changed the title Jest test for bold and italic command Jest test for italic command Jul 4, 2020
@cypherean cypherean mentioned this pull request Jul 4, 2020
5 tasks
@cypherean
Copy link
Contributor Author

Please review @jywarren @emilyashley @keshav234156 @NitinBhasneria @Shulammite-Aso . Thanks!

@keshav234156 keshav234156 reopened this Jul 4, 2020
@keshav234156
Copy link
Member

Restarting Travis!!

Copy link
Member

@keshav234156 keshav234156 left a comment

Choose a reason for hiding this comment

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

LGTM!!

@keshav234156 keshav234156 self-requested a review July 4, 2020 20:25
@keshav234156
Copy link
Member

@shreyaa-sharmaa tests look good and are working well. Can you please document the tests( what each step of a test is doing) so that it becomes easy for the newcomer to understand and implement a similar test for the future.
Thanks!!

Copy link
Member

@keshav234156 keshav234156 left a comment

Choose a reason for hiding this comment

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

LGTM!

@cypherean
Copy link
Contributor Author

Done @keshav234156!!

@cypherean cypherean changed the title Jest test for italic command Jest test for bold and italic command Jul 5, 2020
@jywarren
Copy link
Member

jywarren commented Jul 7, 2020

Awesome, folks!!! Great work!!!!

@jywarren
Copy link
Member

jywarren commented Jul 7, 2020

Awesome work! This is tremendous!

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.

5 participants