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

Add rudimentary tests for ioreg #128

Merged
merged 4 commits into from Aug 28, 2014

Conversation

Projects
None yet
3 participants
@bgamari
Contributor

bgamari commented Aug 7, 2014

No description provided.

// See the License for the specific language governing permissions and
// limitations under the License.
/*! Tests for ioreg! syntax extension */

This comment has been minimized.

@bharrisau

bharrisau Aug 7, 2014

Contributor

Preferring //! over /*!. Not sure if that is a Zinc style thing, but /* */ is falling out of favour anyway.

This comment has been minimized.

@farcaller

farcaller Aug 7, 2014

Member

Yup, that's also what the rust style suggests.

unsafe {
let test: BASIC_TEST = zeroed();
test.reg1.set_field1(true);
assert_eq!(test.reg1.field1(), true)

This comment has been minimized.

@farcaller

farcaller Aug 7, 2014

Member

I'd prefer to see hamcrest/shiny in here, the tests are getting more readable and they're deps already anyway.

This comment has been minimized.

@farcaller

farcaller Aug 9, 2014

Member

Ok, sorry, those are not in tree yet, they are in a dedicated PR now, though: #131. You can extract 0d138bc or just wait for me to merge that one.

This comment has been minimized.

@bgamari

bgamari Aug 15, 2014

Contributor

I've rewritten the set with Shiny. It was a slight improvement although it rendered the line numbers useless upon assert failure which is extremely unfortunate. I also tried Hamcrest although saw no advantage in this case other than introducing more syntactic noise as I only need equality matchers.

In light of this, I'm leaning weakly towards keeping things the way they were as the very slight improvement in readability is arguably not worth the loss in usability of the resulting test output.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 17, 2014

@bharrisau @farcaller any thoughts on the above? As I said, my weak opinion is that we should just keep things simple (yet still quite readable, IMHO). However, if others feel strongly about using Shiny/Hamcrest I'm willing to go that route.

Either way, I would like to start clearing out my patch queue and move on to other things.

@farcaller

This comment has been minimized.

Member

farcaller commented Aug 17, 2014

It may be reasonable to convert hamcrest's expect to expect!, that will take line!() into account. Overall I prefer hamcrest tests as they do add to condition readability as compared to plain assertions (or, uh, I've written to much test code in rspec).

I'm not enforcing both libs, but my two points are:

  • there should be one way we do tests across zinc;
  • shiny/hamcrest really worked out nice in os tests, and I'm cleaning up PT tests to use shiny as well and so far I like the result.

I'll approve this one PR no matter what you do use for tests, but let's get into ML to discuss if we're going to use shiny/hamcrest everywhere until I started doing that on my own. And if we end up using the libs, the code will be refactored.

PS: expect some latency in my responses, out on vacation ☀️

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 17, 2014

r?

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 18, 2014

I'm still away, so I've only given it a quick scan - but it looks good to me.

Looks like Travis is failing on an ioreg related issue. Can you double check that then r=me.

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #128 from bgamari/ioreg-test
Add rudimentary tests for ioreg

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #128 from bgamari/ioreg-test
Add rudimentary tests for ioreg

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #128 from bgamari/ioreg-test
Add rudimentary tests for ioreg

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 18, 2014

Merge pull request #128 from bgamari/ioreg-test
Add rudimentary tests for ioreg

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 19, 2014

Merge pull request #128 from bgamari/ioreg-test
Add rudimentary tests for ioreg

Reviewed-by:

hacknbot added a commit that referenced this pull request Aug 19, 2014

Merge pull request #128 from bgamari/ioreg-test
Add rudimentary tests for ioreg

Reviewed-by:

@bgamari bgamari force-pushed the bgamari:ioreg-test branch 2 times, most recently from c00c200 to b7e2292 Aug 28, 2014

@bgamari bgamari force-pushed the bgamari:ioreg-test branch from b7e2292 to 81119a8 Aug 28, 2014

@bgamari

This comment has been minimized.

Owner

bgamari commented on 0b2b3a7 Aug 28, 2014

r=bharrisau

This passed in https://travis-ci.org/hackndev/zinc/builds/33755318 which included build fixes in hackndev#142 which has yet to be merged.

@bgamari bgamari added ready and removed in progress labels Aug 28, 2014

bgamari added a commit that referenced this pull request Aug 28, 2014

Merge pull request #128 from bgamari/ioreg-test
Add rudimentary tests for ioreg

@bgamari bgamari merged commit a657e06 into hackndev:master Aug 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment