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

Implement test_lesson and test_course functions #15

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ChihChengLiang
Copy link
Contributor

resolving #14

Dear maintainers,

In this pull request the test_lesson and test_course functions are implemented. Thanks to Wush's guidance.

I am also considering writing a tests for these functions by testing a sample course in inst/ dir. Let me know is the idea plausible or is there a better way to do it.

Thanks and best regards,
Chih-Cheng

@seankross
Copy link
Member

Hi @wush978 and @ChihChengLiang,

Swirlify is about to undergo some major API changes, but both of your contributions will be incorporated. I plan to submit swirlify to CRAN by the end of October. If you'd like to appear as contributors to the package please tell me your first and last names so I can add them to the description.

@ChihChengLiang
Copy link
Contributor Author

Thanks @seankross , the names are Wush Wu and Chih-Cheng Liang.

@seankross
Copy link
Member

Hi @wush978 and @ChihChengLiang,

I have a suggestion. Perhaps instead of stopping, the user should be notified which question doesn't pass the test, and then the testing should continue. If you don't have time to implement this change I understand and I'll get around to it.

@ChihChengLiang
Copy link
Contributor Author

Dear @seankross ,

The suggestion is really interesting! Unfortunately, we have no time to implement the feature in this month. Please don't hesitate to continue the awesome work.

@seankross
Copy link
Member

Hi @ChihChengLiang and @wush978,

I've merged this into the development version of swirlify, but right now there are some corner cases for this scheme of testing that worry me. For example: if the student creates a function during the lesson and then is instructed to use that function in a command question later on, how will the environment be aware of that function? Another detail is the use of ::: which I will soon be allowed to use, however I have to wait until I become the official maintainer of swirl on CRAN which won't happen until the next release of swirl. All of this being said I'm going to keep this PR open but it requires more thought. However I certainly believe that swirlify needs this feature!

Sean

@wush978
Copy link

wush978 commented Nov 13, 2015

Dear Sean,

Thanks for your feedback.

In general, I think the best strategy (for a volunteer) is to keep updating this feature if we find any new corner cases. Recently I extend this feature to test script question and I'll update it. Keep it opened should be a good idea.

For example: if the student creates a function during the lesson and then is instructed to use that function in a command question later on, how will the environment be aware of that function?

In my opinion, the question is "how will swirl do if the user skip the cmd_question?" This test feature assumes that the swirl knows how to do if the user press skip() and makes sure that skip() will not crash the course. That is to say, the author should provide the answer of the function once the user skip the cmd_question. If the author does not provide the answer, then failure of the test indicates that the swirl will crash if the user skip the cmd_question.

@seankross
Copy link
Member

@wush978 I agree 100%. I truly appreciate the volunteer work that both of you do. I think this will be the main feature of swirlify 0.5 but it won't make it into swirlify 0.4.

@wush978
Copy link

wush978 commented Nov 17, 2015

Hi @seankross ,

I just add the script for testing script course to the PR.

Moreover, I am trying to use our test script to check the course in https://github.com/swirldev/swirl_courses. Here is a prototype of checking: https://travis-ci.org/wush978/swirlify/builds/91589457. Do you think if we could check the courses in there and make them all pass the test script?

@seankross
Copy link
Member

Hi @wush978,

This is great! Integrating travis to check courses is really good idea. Thanks for working on this!

@wush978
Copy link

wush978 commented Nov 18, 2015

Dear @seankross,

I guess the latest update could solve your problem in #15 (comment).

As shown in https://travis-ci.org/wush978/swirlify/builds/91776480#L2537, the lack of boring_function will trigger an error in unit test. The boring_function is defined in the boring_function-correct.R , so this bug should be correct after merging ChihChengLiang@a485769#diff-859757758e406f7d4bd75ab7590a97b8R502 into branch 3.1

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