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

Modified jasmine script #1101

Merged
merged 3 commits into from Jun 7, 2019

Conversation

@aashna27
Copy link

commented Jun 6, 2019

jasmine

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-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

Thanks!

@aashna27 aashna27 changed the title Jasmine Modified jasmine script Jun 6, 2019

@jywarren
Copy link

left a comment

Was this not working properly before? What changed? Thanks!!

@codecov

This comment has been minimized.

Copy link

commented Jun 6, 2019

Codecov Report

Merging #1101 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1101   +/-   ##
=====================================
  Coverage   56.3%   56.3%           
=====================================
  Files        110     110           
  Lines       2275    2275           
  Branches     357     357           
=====================================
  Hits        1281    1281           
  Misses       994     994
@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Was this not working properly before? What changed? Thanks!!

No it wasn't the jasmine wasn't found, but I would appreciate if someone verified it on their system as well.

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Also @jywarren I am a little lost as to what type of tests should I make to test the UI, I am new to testing so I wanted to start with something very basic and was thinking of using jasmine to test individual functions that we have to see if they have proper results. Please provide some guidance, as to what can be the best approach.

@jywarren

This comment has been minimized.

Copy link

commented Jun 6, 2019

@jywarren

This comment has been minimized.

Copy link

commented Jun 6, 2019

Hmm, and why is this not affecting the coverage? Curious!

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Hi! Well, one thing you can start with is to think of the actions you take in using the sequencers html UI, and thinking of using jQuery to actually send click and change events to each of the buttons and menus you use - then checking to see if the correct things actually happened, whether you're checking if the sequencer has more or fewer steps, or if the HTML interface actually shows the correct response. This can help to test all features working in concert in a real world scenario, and is sometimes called integration testing. This is the opposite type of testing to unit testing, which would be to try to consider and test each individual function out of it's context, but just to confirm that it fulfills it's most basic purpose apart from a complete working system. This can help narrow down the reasons for bugs by testing each thing separately. Both types of test are valuable for different reasons!

On Thu, Jun 6, 2019, 1:47 PM aashna27 @.***> wrote: Also @jywarren https://github.com/jywarren I am a little lost as to what type of tests should I make to test the UI, I am new to testing so I wanted to start with something very basic and was thinking of using jasmine to test individual functions that we have to see if they have proper results. Please provide some guidance, as to what can be the best approach. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1101?email_source=notifications&email_token=AAAF6J4QWVC5PB6EBSZ4YALPZFZUXA5CNFSM4HVJGL22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXEDUYQ#issuecomment-499661410>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAF6J7H5QYFW7IFXRBGNDLPZFZUXANCNFSM4HVJGL2Q .

Yeah will go with unit testing first. Thanks

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Hmm, and why is this not affecting the coverage? Curious!

I checked the travis logs for this pr and the past ones and the earlier script works there. Although it wasn't working for me, if someone else can verify that the previous one works we can dismiss this change.

@HarshKhandeparkar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@aashna27 I have handled this in my $step PR in which instead of modifying the script location, I just changed the setup script to install jasmine locally

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

Yeah, but i tried it was even happening after locally installing jasmine so i read the docs and it said to use the node_modules path for jasmine.

@HarshKhandeparkar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

It worked for me though.

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

@HarshKhandeparkar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

My method worked for me. This should work but I haven't tested yet. Should I?

@HarshKhandeparkar
Copy link
Member

left a comment

@aashna27 works for me!

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

@HarshKhandeparkar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Travis installs jasmine globally. Oh wait, we don't need that now right? Can we remove it then?

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

Remove what?

@HarshKhandeparkar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

remove the travis script part which installs jasmine

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

remove the travis script part which installs jasmine

Ohh right, travis installs it globally, yes it can be removed lets keep it local. 👍 I ll do it with this one then. Thanks !

@jywarren jywarren merged commit c653ce6 into publiclab:main Jun 7, 2019

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jywarren

This comment has been minimized.

Copy link

commented Jun 7, 2019

Ok!! Thanks for figuring this out!!

@aashna27

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

Thanks @HarshKhandeparkar and @jywarren

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.