Skip to content
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

New tests are required #173

Open
vkostyukov opened this issue Apr 21, 2014 · 12 comments
Open

New tests are required #173

vkostyukov opened this issue Apr 21, 2014 · 12 comments

Comments

@vkostyukov
Copy link
Owner

In order to test new operations new tests should be introduced. We have to test all the possible combinations of pair: Sparse-Dense. New tests should also test inPlace operations.

@vkostyukov vkostyukov added this to the la4j-0.5.0 milestone Apr 21, 2014
@vkostyukov vkostyukov added the bug label Apr 21, 2014
@ilya-murzinov
Copy link

@vkostyukov, hello!
I am looking for some jump-in issue to start contributing to la4j.
I think this issue is sutable for my purpose, am I right?

@vkostyukov
Copy link
Owner Author

Hi @ilya-murzinov, that's correct! New tests are something that is needed right now. I'm in the middle to introduce new operations, so we have to be able to test them.

I would suggest you to start with infrastructure. There are already tests in src/test package that tests paired operations such as testing the linear-system-solver. We somehow have to understand how to write a single test that take a couple of factories/vectors and launch it with all the possible combinations of factories.

This is not an easy issue but I believe it's possible. Please feel free to ask any questions in this issue and submit your PR either in operations or master branches.

@vkostyukov
Copy link
Owner Author

Hi Ilya @ilya-murzinov,

Plsee see the issue #174 for more details. I think it would be a good idea handle these issues together in order to not do the same work twice.

@ilya-murzinov
Copy link

@vkostyukov, ok
Right now I'm in the middle of "What the hell is that?!" stage, but I'm trying to figure out how is everything working here 😄

@vkostyukov
Copy link
Owner Author

Sure! I'm not a big expert in writing tests that's for sure. So, we can turn it in any direction we want. That is a good time for changes in a whole test package. As a contributor you a have a great chance to design everything from scratch.That is not exactly the the jump-in issue anymore. But I think it's more challengeable now.

@ilya-murzinov
Copy link

@vkostyukov, I have a good experience in software testing (I'm actually QA automation specialist right now), but I can't say that it's gonna be easy for me. I can only promise to do my best :)

If we want to start from scratch, it's a good idea to create a branch for it and just remove test folder there completely. Then we can start.

@vkostyukov
Copy link
Owner Author

@ilya-murzinov,

Right. I'm ok with rewriting everyting. The only thing I care is all the tests that are already in la4j. Some of them are good regression-tests. We somehow have to migrate tests to the new infrastructure rather then inventing new test-cases.

@ilya-murzinov
Copy link

@vkostyukov, yes, of course, I am going to base new tests on old ones.
First thing I'm donna do is to figure out how tests are working right now and then try to rewrite them one-by-one.

@vkostyukov
Copy link
Owner Author

@ilya-murzinov that's a nice plan.

Tests are primitive. Everything is bult around factoires (that can produce vectors/matrices). There are 4 factories: CRS (Compressed Row Storage), CCS (Compressed Column Storage), Basic1D, Basic2D that might generate 4 corresponding matrices and 2 vectors Compressed and Basic.

The current implementation is based on a simple idea having an abstract test classes (i.e., AbstractVectorTest) with 4 child classes that overrides a factory() method withit a corresponding factory instance. This works fine but it doesn't test the combinations of factories (like Sparse-Dense), which is becoming important, since new operations are cover these cases.

@ilya-murzinov
Copy link

@vkostyukov I'm very sorry, but I don't think I'm capable of solving this issue now. I'm experiencing lack of motivation for such a big thing. May be I should try to do something smaller for the start.
Again, I'm very sorry for letting you down :(

@vkostyukov
Copy link
Owner Author

Hi @ilya-murzinov,

No worries! How about take a look at the issue #172. Just refactor the methods to use a proper agument times. Also, don't forget to mark the old methods depricated.

@SamoylovMD
Copy link
Contributor

According to #216, new equals() methods in AbstractVector and AbstractMatrix should be tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants