When docker isnt present #343
Comments
|
Some discussion before I implement and PR? |
|
IMO letting the test fail is the correct behavior. |
|
If the test was actually to run docker I would agree. As an example. I'm using JDBCContainer to spin up a mysql db and then performing some operations. Should I assume that the operations are failing which is the actual test? jUnit Assume would mark them as ignored and as such would not require everyone working with the project to have docker installed. You just make sure that the CI env has docker. |
|
Thinking the same way: what if MySQL is not available for any reason ? Do you expect your tests to fail or to be ignored ? I agree with @kiview that failure is the correct behavior. |
|
Might have been unclear, since I don't want a breaking change I just want the ability to do something like: And this is since there is no good way of ignoring test that you know are going to fail. |
|
The whole idea behind assumptions is that tests should not fail just because you made an assumption. They will not pass, they will not fail. And adding this to ones project just makes the assumption that people aren't on Windows and that everybody has docker installed. (it does work on windows with limitation which seesm to be related to Compose, got the information from the slack channel) Letting me as the user control this assumption would be a great thing IMO. |
|
If the ability to assumeDocker or ability to create assumptions easy on your own is not wanted this can be closed. I however strongly feel that failing a DAO test because of container failure or missing docker is wrong. |
|
Let's wait for the opinion of @rnorth. |
|
Thanks all, and I'm sorry I haven't replied on this thread until now.
It's an interesting suggestion; I've used Assume once or twice but not come across use in the wild much, so you're the first person to ask for this. I can see how you'd find this useful, though. I can also see how Testcontainers has unique information about the Docker environment that is useful for such a Assumption.
It won't be something that everyone wants to use, so I'd like to find a way to do it that's not too far-reaching. How about this:
Could we perhaps proceed to do this separately from the container classes? E.g. Create something to allow either `assumeTrue(DockerClientFactory.isDockerAvailable())`, or `DockerAvailableAssumption.assumeDockerAvailable()`?
Another reason for keeping it off the container classes is that I'd like to separate out any JUnit coupling from the container classes, so adding an assume method right now would seem to be a step in the wrong direction.
Thoughts on the above approach?
Richard
|
|
@rnorth Since I have my fork and jitpack exists, this is not much of an issue. |
|
Depends on what is considered "in the wild". |
|
@npetzall Yes, I think this assumption should be removed. I see no reason this test to fail on Windows. |
|
@martin-g I don't care anymore. But I agree with you "If Docker is not there then the whole test has no meaning" ergo Ignored, since Pass/Fail both have meaning. |
|
@rnorth |
|
That sounds like a good idea and it's something we were already discussing, since we want to decouple TestContainers and JUnit (and there are some more parts in the codebase that would profit from using composition instead of inheritance). And once we've decoupled, I'd personally be fine to add more JUnit specific stuff to the JUnit part of the API. AFAIK it's planned for version 2.0, but we might need to have a discussion regarding the final design first. |
|
@kiview So really this can be implemented now, since it will later be refactored and moved to a junit module? |
|
If schemaspy otherwise has to fallback to using the Jitpack build of it's own fork of TestContainers, it might be the more sensible approach to have some trade-off solution at this point in time. Depending on the current progress of the refactoring, I'd be fine with it, but I think @rnorth or @bsideup should decide, since they are more involved in the core. |
|
The fork also solves #344, #345 so only doing this will not super-seed the fork. |
Really nice attitude. But then again why is a fork a problem. Isn't the whole point that if we have different opinions in regards to the code, one can make a fork and customize it to ones liking? To me it just seems silly that you don't want the behaviour. I fork and suddenly it's a bit interesting. |
|
@npetzall all we're asking is not to add another JUnit-related method to GenericContainer. As I said, we can see the value of what you're asking for, but please try and understand that for us this is something we have to maintain in the long term. If we add another JUnit-related feature to a core class then that just increases the likelihood of other users using it (not just yourself), and suffering when we move it (as we plan to do). We're not ready to refactor JUnit out yet, so this is just the position we're in. My suggested approach is this to have this in relevant tests for JUnit 4: I have a working implementation of this sitting right in front of me, and I think we can easily include it. If you don't want to use it, I can't stop you, but I would be grateful if you could adopt the approach that we're comfortable with supporting. |
|
@rnorth I can't use it due to issue 344. Also could you do the following test for me then. So order is somewhat unimportant. Turn of docker so that It isn't present and run the test. This is because construction of GenericContainer does to much. It fails when creating the MysqlContainer. The AssumptionRule might not even be evaluated. As for the refactoring out. You can add |
|
You should use RuleChain when you need to order your rules
…On Jun 3, 2017 10:58 PM, "Nils Petzäll" ***@***.***> wrote:
@rnorth <https://github.com/rnorth> I can't use it due to issue 344.
Also could you do the following test for me then.
Order of rules is undetermined according to junit javadoc for @rule and
@ClassRule Rule <http://junit.org/junit4/javadoc/4.12/org/junit/Rule.html>
[ClassRule] (http://junit.org/junit4/javadoc/4.12/org/junit/ClassRule.html
)
So order is somewhat unimportant.
@ClassRule
public static MysqlContainer mysqlContainer = new MysqlContainer<>();
@ClassRule
public static AssumptionRule assumptionRule = AssumptionRule.assumeDockerPresent();
Turn of docker so that It isn't present and run the test.
You might get some ignored test, but that's only because you get an error
java.lang.ExceptionInInitializerError.
This is because construction of GenericContainer does to much. It fails
when creating the MysqlContainer. The AssumptionRule might not even be
evaluated.
As for the refactoring out. You can add @deprecated to the junit stuff in
core right now and create the junit module. There is no reason why they can
not co-exist.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#343 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOKQl2IqwYtnGDbcy8u_gscLFvc7Pysks5sAbrlgaJpZM4NjOZ8>
.
|
|
@martin-g that's correct it says so in the linked Javadoc. But it doesn't matter in this case since the test fails due to inability to instantiate the field that has the |
|
@rnorth To take a step away from junit in core i've done this: issue_343_junit New module junit module with GenericContainerRule which handles assumptions and such. It will allow you to add an assumption on the JdbcDriver as well. Which will avoid start of the container. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
|
This issue has been automatically closed due to inactivity. We apologise if this is still an active problem for you, and would ask you to re-open the issue if this is the case. |
|
I'd maybe suggest to start with the most basic thing: a way to check whether Docker is actually available or not, as a simple static boolean function, e.g. |
|
There may be another use case to consider as well: someone wants to (maybe just temporary) deactivate testcontainers in his build see --> #2833 (comment) |
|
We (Eclipse Jetty) build and test on a huge variety of OS's, architectures, and hardware combinations. Can we at least get a unique exception like Currently, we get an |
|
@joakime that's an interesting idea. Also, have you considered failing fast by checking |
|
@bsideup I can report that the following junit5 behavior works on the various architectures and OSs that have proven difficult. (think ARM64 and new/experimental/alternative OS's) @Test
public void testSomethingThatNeedsDocker() {
Assumptions.assumeTrue(DockerClientFactory.instance().isDockerAvailable(), "Docker is not available on this environment");
// do something that needs docker here
}It's a shame that the docker resolution occurs twice though. |
if code makes use of GenericContainer or any other TestContainer based rule then those rules will mostl likely throw an exception when docker is not available. This leads people to add a BeforeClass annotation and test for docker, however this does not skip the tests, it skips the class. This has the unfortuante side effect that the report (from thinks like maven surefire and ant) that the test did not run when ingested into tools like Jenkins (as zero tests from the class either passed failed or skipped). it is also not obvious on the command line. by using a rule the individual tests will be marked as skipped so it becomes obvious that a test existed, but did not run for some reason (and the reason will be in the exception - that docker is not available) fixes testcontainers#4586 / testcontainers#343
Would be nice if one could tell the subclass of GenericContainer that one is assume-ing that docker is present. So that if someone that doesn't have docker, get ignored tests instead of failing tests.
Due to the fact
DockerClient is instantiated during of GenericContainer. One has no ability to use the jUnit way of conditionally ignoring tests with assumThat and AssumptionExceptionViolation.
Instead one gets:
So create the DockerClient later should make it possible to use assumptions if one would like to.
Also noticed that there is a getName after setting name that causes issues with the creation of the object. One would say it's doing to much in its construction, there is a "start" phase.
The text was updated successfully, but these errors were encountered: