classic icon indicating copy to clipboard operation
classic copied to clipboard

Adding tests, rockspec file, and TravisCI integration

Open subnetmarco opened this issue 9 years ago • 4 comments

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 avatar Dec 20 '14 04:12 subnetmarco

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 avatar Jan 06 '15 23:01 subnetmarco

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

subnetmarco avatar Mar 10 '15 01:03 subnetmarco

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

rxi avatar Mar 11 '15 20:03 rxi

@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: https://github.com/thefosk/classic/commit/1041afa251f644afb58028badf4be4bee74eee7e#diff-a5bbebb071dab977909b22acfa4e1563R47

subnetmarco avatar Mar 11 '15 21:03 subnetmarco