Review about grounding-search HOT 12 CLOSED

Yes, mocha tests have a default timeout that is low. How about this solution: The tests build the index only if an env var is set, e.g. TESTS_BUILD_INDEX=true. The sample target can set the variable. Because it's false/unspecified by default, the main test target assumes that the index is already built before starting the tests.

I think in this case if TESTS_BUILD_INDEX is not set we will be skipping the tests for loading and clearing data. Then npm run test will only be testing the operations that does not modify the data, such as search and get. If someone want to test the functions that modify data then he can use npm run test:sample. It makes sense to me.

from grounding-search.

Don't use NODE_ENV=test. Make the ci script set env var for the local uniprot test file. Maybe rename the ci script to travis if you want.

All of the other items in todo list are fair to me but I am not clear about this one.

• If I set env var for the local uniprot test file in ci script then npm test command will not be working as desired by itself. Test will be working fine for just the ci tool. However, if I do it from test script then both will work fine (Am I wrong?). So, why do I need to do it from ci instead of test?
• For test not only the input file path but also the index name used in elasticsearch is different (The purpose is not to override/modify the real data). Therefore, setting env var for the local uniprot test file will not be enough by itself. I will also need to set another env variable for index name in that case.

from grounding-search.

If I set env var for the local uniprot test file in ci script then npm test command will not be working as desired by itself. Test will be working fine for just the ci tool. However, if I do it from test script then both will work fine (Am I wrong?). So, why do I need to do it from ci instead of test?

The test should use the normal uniprot env var. If you want to use a local file, point to a local server URL with the env var, e.g. using npm i -g http-server.

For test not only the input file path but also the index name used in elasticsearch is different (The purpose is not to override/modify the real data). Therefore, setting env var for the local uniprot test file will not be enough by itself. I will also need to set another env variable for index name in that case.

Yes, that extra var makes sense.

from grounding-search.

The test should use the normal uniprot env var.

Loading actual uniprot data takes a lot of time. I think that tests will not be practical to use with the actual data. I can set evn variables for file path and database index while running npm test from cli but why we are not using them (test file and test index, one other concern is modifying the actaul db index may not be good) by default as we do for ci. Because, as I mentioned tests will not be practical with actual data (also we need to remove timeout from load data test if we will be using the actual uniprot data)

from grounding-search.

The main point to consider is that default targets should do default things: npm test should run with the normal env vars. We can have separate targets, like npm run test:sample or npm run test:travis that set env vars to use smaller data sets etc.

from grounding-search.

The main point to consider is that default targets should do default things: npm test should run with the normal env vars. We can have separate targets, like npm run test:sample or npm run test:travis that set env vars to use smaller data sets etc.

Okay

The test should use the normal uniprot env var. If you want to use a local file, point to a local server URL with the env var, e.g. using npm i -g http-server.

Okay then it will be required to run the server in the default port (or another specific port) to run a command like npm run test:sample since port can also be controlled through an environment variable.

from grounding-search.

Basically a target like test:sample should set all env vars necessary to make the basic tests run on a dev machine, and especially on Travis CI.

from grounding-search.

@maxkfranz I did the steps expect enabling travis. For travis I have insufficient permissions warning.

from grounding-search.

About the npm run test command. I see the concern about running the test command with actual environment variables but there is a fact that test command is useless the way it is implemented now. It is useless because it will always be timed out.

How about doing one of the followings?

• Removing npm run test and having only npm run test:sample
• Keeping npm run test but having no timeout or setting timeout large enough where it is needed (I am actually still worried about this one because no developer would expect to wait that much for the tests and since it will be playing with actual index if tests are manually interupted in halfway it may cause loosing the index which will take a considerable amount of time to reload).

from grounding-search.

@maxkfranz I did the steps expect enabling travis. For travis I have insufficient permissions warning.

I enabled it and added the badge. Now the tests and linting have to pass for the badge to become green.

from grounding-search.

About the npm run test command...

Yes, mocha tests have a default timeout that is low. How about this solution: The tests build the index only if an env var is set, e.g. TESTS_BUILD_INDEX=true. The sample target can set the variable. Because it's false/unspecified by default, the main test target assumes that the index is already built before starting the tests.

from grounding-search.

Exactly!

from grounding-search.