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

Test for DecodeQR Module #584

Closed
wants to merge 4 commits into from
Closed

Test for DecodeQR Module #584

wants to merge 4 commits into from

Conversation

vishalbakshi
Copy link

@vishalbakshi vishalbakshi commented Dec 31, 2018

Fixes #546

I have created a test for the DecodeQR Module in the test/modules/decodeqr.js file in my repo (vishalbakshi/image-sequencer > main)

This is my first time working with GitHub, as well as writing a test for an open source project so please let me know if I have not followed any common-sense conventions.

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 rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • 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

Thanks!

@welcome
Copy link

welcome bot commented Dec 31, 2018

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

@jywarren
Copy link
Member

jywarren commented Jan 2, 2019

This is great!!! I'd love to see it tested in the context of a sequencer "run" -- like we've done here:

test("Decode QR module works properly :: setup", function(t) {
sequencer.loadImage(qr, function() {
this.addSteps('decode-qr').run({ mode: 'test' }, function() {
t.end();
});
})
});
test("Decode QR module works properly :: teardown", function(t) {
t.equal("http://github.com/publiclab/image-sequencer", sequencer.images.image2.steps[1].output.data);
t.end();
});

Perhaps we could combine these lines with your standalone test file? Have you confirmed this is being run in our Travis test runner here on GitHub?

Great work! Thank you!

@vishalbakshi
Copy link
Author

Awesome---thanks!!

The following Travis CI output was received after I opened the PR:

https://travis-ci.org/publiclab/image-sequencer/builds/473963530?utm_source=github_status&utm_medium=notification

Additionally, it shows "All checks have passed" for my "main" branch (screenshot below).

screen shot 2019-01-01 at 10 30 28 pm

Please let me know if there is something else with Travis that should pass.

I will work on combining my test file with the structure of the image-manip.js file you have referenced--thanks!

@jywarren
Copy link
Member

jywarren commented Jan 3, 2019 via email

@vishalbakshi
Copy link
Author

Hi @jywarren:

I have a run into an issue as I try to incorporate my standalone code into image-manip.js:

  1. Process is stalling

    • When I try to run the following:

    test("Preload", function(t) {
    sequencer.run({ mode: 'test' }, function() {
    t.end();
    });
    });

    • The process stalls for me, specifically at the while loop in Run.js in the filter function definition in Run.js:

    while (
    typeof prevstep == "undefined" ||
    typeof prevstep.output == "undefined"
    ) {
    prevstep = ref.images[image].steps[--json_q[image] - 1];
    }

Since the value of image is 'test' and the value of json_q[image] remains undefined. I believe this value is being set in ImageSequencer.js in the run function definition:

arguments['0'] = config.mode;

However, I'm not sure how to get around this while still using the { mode: 'test' } object. If this is an issue that only I am experiencing then let me know and I'll continue digging.

  1. Calling run without the { mode: 'test' } config object
  • I have incorporated my tests into image-manip.js and they all pass, but I am not sending { mode: 'test' } to any of the run functions. Please let me know if I should still commit that file to my repo or wait until I get it to work with that config object.

  • Additionally, and maybe more importantly, I am not sure if my tests follow the same philosophy as the existing tests since they are more closely emulating a sequencer run while mine deviate from that.

Please let me know if you have feedback on any of the above points.

Updated:
`const decodeQR = require('../../src/modules/DecodeQR/Module.js')`
to
`const decodeQR = require('../../src/modules/DecodeQr/Module.js')`
in order to attempt to pass the Travis CI tests.
@jywarren
Copy link
Member

Hmm, this is interesting! The tests seem thorough, but i see what you mean about them being kind of different than other tests. Why don't we ask someone from @publiclab/is-reviewers to read through this and offer their thoughts too? Thank you!

@jywarren jywarren requested a review from a team January 12, 2019 04:18
@harshkhandeparkar
Copy link
Member

Issue fixed

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.

3 participants