-
Notifications
You must be signed in to change notification settings - Fork 29
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
Tests don't pass on fresh Master pull #91
Comments
This particular test relies on there being storage backing for the namespace. I suspect the namespace you tested against was configured If so, the test should have checked the namespace's configuration and expected 0 instead. |
* Fixed device Size test for not in-memory namespaces (#91) * Fixed HLL returns in tests * Lowered ms on since_update to prevent failing
@brianbruggeman I believe the failing tests have been addressed in #93. I'm still waiting for the latest CI results, but I'm going to close this for now. The tests already pass on my local dev env. Please reopen the issue, if any tests are still failing for you. @jonas32 also mentioned that he would be addressing the Clippy warnings in another PR. |
@jhecking @kportertx Where is a good space to discuss things? |
I pulled the latest.... I'm still probably doing something wrong:
|
@brianbruggeman what version is the Aerospike Server you used? |
@jonas32 I included the docker command I'm using above so there wasn't any confusion. That said, it only pulled "latest" rather than an explicit version. And since it's hard to know what version aerospike is, I've removed that image and repulled as: aerospike-server:5.4.0.2 (which should be latest from dockerhub). Notably, latest appears to be newer than 5.4.0.2 The configuration template used can be found here in the github repo for aerospike's docker.. Note that these two links are specific to 5.4.0.2. I ran the test on 5.4.0.2 (see below). I am not entirely sure what the configuration looks like in the docker container, but I'm assuming that whatever it is, there's nothing special that I need to do to run the tests.
And just for clarification:
|
I guess your namespace has expiration enabled. The test assumes its turned off. If your namespace does expiration, the test will fail because we are checking for 0 as expiration time. |
Okay. Thank you for the explanation there. It looks like I can modify my docker run invocation setting the
And of course now the standard tests pass as you have expected:
However, cargo test has now started to run on the doc-tests and they appear to also be failing. I assume that those are expected to pass as well? I'll take a look at the travis CI configuration file here and see if I can duplicate everything within the docker image. That said, from an outsider looking in, it appears that this crate has a very special non-standard configuration (a snowflake) that is required for testing. And that makes me feel uneasy when considering if I can rely on this crate in production. |
I fully understand that. Can you please share more about the failing doc tests? |
One option would be to create the test records with a specific TTL, instead of relying on the default value. But even that might be problematic, because as of server version 4.9 record expirations are disabled on the server by default. That's also why the test config includes Testing the |
This is how I solved the problem in for the Aerospike Node.js client test suite: Before any of the tests run, the test suite fetches the server config for the test namespace. For the tests that use the This is all still a work-in-progress, but you can see the relevant changes here: aerospike/aerospike-client-nodejs@2cd6f86 |
@jhecking does your CI then run tests on both modes? |
Note: I'm using rustc 1.49.0 (e1884a8e3 2020-12-29) for the tests below. Using your branch for PR #94, I ran with the docker command I had above. I did have an initial failure which was resolved when I reran:
Doc-tests:
|
It looks like many of these are probably because of a missing environment variable. Is there any opposition to adding dotenv to dev-dependencies and then using For example:
|
Why do you want to add dotenv? Its taking the vars from env with default fallback. static ref AEROSPIKE_HOSTS: String =
env::var("AEROSPIKE_HOSTS").unwrap_or_else(|_| String::from("127.0.0.1"));
static ref AEROSPIKE_NAMESPACE: String =
env::var("AEROSPIKE_NAMESPACE").unwrap_or_else(|_| String::from("test")); |
Because |
We are talking about 2 variables. Why do you want to set them that way? You dont have to set it globally. You dont even have to do it at all actually because the default value will take the place. And if you want to overwrite that, just execute it with |
@kportertx suggested that this was an open community, but now I'm wondering if that's really true. I hate wasting time playing unnecessary politics on something that I'll end up just forking and then building out what I need so I can get something done. While I already provided a decent explanation: You don't have to use it personally. As a developer who works on a bunch of different projects, I really appreciate having a local |
This is indeed community driven. Im not related to Aerospike in any way... |
I think optionally reading a |
@jhecking the client technically did not include the package - only the dev-dependencies which are explicitly for testing and things like documentation. I think the makefile gets around the limitation, but if someone knew Rust and expected "cargo ..." to work, then it wouldn't without extra hassle (though obviously, |
Understood, and I would be open to using dotenv as a test dependency, though I'm not sure it's still needed since your Makefile covers that use-case quite well. I just don't think the client itself should have any dependency on |
I'm running macOS 10.14.6.
Reproduction Steps:
Results:
Running cargo clippy:
The text was updated successfully, but these errors were encountered: