search

coblox / nectar

GNU General Public License v3.0
0 stars 1 forks source link

Add "docker-test" feature to flag to allow testing on mac os #8

Closed D4nte closed 4 years ago

D4nte commented 4 years ago

This way, we can still test most on the codebase of mac. Any test using docker should now be protected behind the test-docker feature.

D4nte commented 4 years ago

Nice! Much better. Do we already do this for comit-rs? Seems useful.

No we don't and yes we should :)

D4nte commented 4 years ago

@thomaseizinger I expect you to enjoy that.

D4nte commented 4 years ago

Well, actually it does not work as intended! The docker tests never run now.

thomaseizinger commented 4 years ago

@thomaseizinger I expect you to enjoy that.

Expectation fulfilled.

thomaseizinger commented 4 years ago

Nit: I liked the naming in your PR title better :)

D4nte commented 4 years ago

See https://github.com/coblox/nectar/pull/9

D4nte commented 4 years ago

Well, testing on my local macbook work, the real issue is testing in github action, on mac, because of docker.

thomaseizinger commented 4 years ago

Well, testing on my local macbook work, the real issue is testing in github action, on mac, because of docker.

You might want to flip the use of the feature around and not make it the default. Features accumulate in cargo so opting-in to something is always super easy. Opting-out is not :)

Even if it doesn't matter much in a binary crate like this, I think it is better to not get into a habit of having default features like this.

D4nte commented 4 years ago

Well, testing on my local macbook work, the real issue is testing in github action, on mac, because of docker.

You might want to flip the use of the feature around and not make it the default. Features accumulate in cargo so opting-in to something is always super easy. Opting-out is not :)

Even if it doesn't matter much in a binary crate like this, I think it is better to not get into a habit of having default features like this.

I do not expect anyone to opt out of this option except for developers who have this code and are making a fork of it and are running a CI with similar issue. Making it opt out will mean that @da-kami and I (mac users) would have to opt in everytime we run the test locally. An inconvenience with no added value.