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

Adding tests, rockspec file, and TravisCI integration #4

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

Conversation

subnetmarco
Copy link

This pull request includes a rockspec file for the project, and two tests built with busted that can be extended in the future. It also includes a .travis.yml file for Travis CI, and an updated README.md file that shows the status of the latest build.

Enabling Travis CI

In order to enable the Travis CI builds, a TravisCI webhook needs to be applied to the repository. That will trigger Travis CI on every push to the master branch. Once this is done, the first line of the README.md file needs to be updated too, to point to the proper build image, so that this text:

[![Build Status](https://api.travis-ci.org/thefosk/classic.png)](https://travis-ci.org/thefosk/classic)

becomes:

[![Build Status](https://api.travis-ci.org/rxi/classic.png)](https://travis-ci.org/rxi/classic)

@subnetmarco
Copy link
Author

Sorry for the commit mess. To recap, what this pull request does is just adding rockspec support to the project, with some basic testing. I brought back the classic.lua file to its original state, exactly as you wrote it.

By merging this pull request the project could also be easily distributed on https://rocks.moonscript.org, which is my ultimate goal. After merging this pull request TravisCI needs to be enabled as specified in my first message.

@subnetmarco
Copy link
Author

hi @rxi, did you have any chance to review this pull request?

@rxi
Copy link
Owner

rxi commented Mar 11, 2015

Hey,

Sorry for the slow response!

Would you mind closing this pull request and instead submit the rockspec addition as a separate pull request, ideally with a cleaner commit history?

Although I'm happy with the idea of the library including some tests, I would prefer they used mixedCase rather than snake_case (as to match the formatting in the project's examples). The use of "say" functions returning a string also doesn't seem to sit right, as I feel it implies the string is printed rather than returned. The test which is listed as using a static method call the method using the : operator rather than the . operator, thus are treated as non-static methods as the self variable is still passed to them despite being ignored by the function -- in fact, it feels as if these functions should take some argument to assure they are being called correctly without a self. Also on line 28 it seems strange to return a table with a message rather than the string itself.

I may be more comfortable writing some small tests myself, as it is, due to my unfamiliarity with busted, I wouldn't be able to add any additional tests to the project if I wanted to in the future. I would likely opt for doing testing the same way I do with lume.

Thanks

@subnetmarco
Copy link
Author

@rxi I have implemented all but one of those changes and cleaned the commit history without needing to create a new pull request.

The only thing that I didn't implement is replacing busted with another library, busted is a very powerful testing library and provides lots of built-in features for handling complex scenarios. For example the check to make sure no arguments are being passed to the static methods has been implemented with Spies: subnetmarco@1041afa#diff-a5bbebb071dab977909b22acfa4e1563R47

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.

None yet

2 participants