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

Remove early return on startup.cpp when testing is enabled #169

Open
lobis opened this issue Mar 28, 2022 · 7 comments
Open

Remove early return on startup.cpp when testing is enabled #169

lobis opened this issue Mar 28, 2022 · 7 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@lobis
Copy link
Member

lobis commented Mar 28, 2022

Originally startup.cpp caused problems with unit testing and to fix this we added an early return if testing was enabled:

#ifdef REST_TESTING_ENABLED
        return;
#endif

This doesn't seem to affect the running of tests, but makes the framework behave differently when testing is enabled, this means that users should not use the framework with tests compiled to produced results etc. and only use it to run the tests, then compile it again without tests. This is also true for the pipeline where in order to run tests the framework is compiled twice.

There should be a way to modify this file so that tests can be run and also the file keeps working as intended, so that we can avoid recompiling twice.

@lobis lobis added the help wanted Extra attention is needed label Mar 28, 2022
@nkx111
Copy link
Member

nkx111 commented Mar 28, 2022

If returned, the global variables REST_COMMIT/REST_PATH/REST_USER are not set. It will make REST behave differently. I think we should set these variables manually before early return under testing condition.

#ifdef REST_TESTING_ENABLED
    REST_COMMIT = xxxx;
    REST_PATH = xxxx;
    REST_USER = xxxx;
    REST_USER_PATH = xxxx;
   return;
#endif

What are the anticipated values of these variables?

@nkx111
Copy link
Member

nkx111 commented Mar 28, 2022

I am not familiar with ctest. Seems that we can input c++ definitions to the code during the test. Then maybe it is also possible to do something like:

#ifdef REST_TESTING_ENABLED
    REST_COMMIT = REST_TESTING_VAR_COMMIT;
    REST_PATH = REST_TESTING_VAR_PATH;
    REST_USER = REST_TESTING_USER;
#endif

where REST_TESTING_XXX are all the imported c++ definitions from the test system.

@lobis
Copy link
Member Author

lobis commented Mar 28, 2022

If returned, the global variables REST_COMMIT/REST_PATH/REST_USER are not set. It will make REST behave differently. I think we should set these variables manually before early return under testing condition.

#ifdef REST_TESTING_ENABLED
    REST_COMMIT = xxxx;
    REST_PATH = xxxx;
    REST_USER = xxxx;
    REST_USER_PATH = xxxx;
   return;
#endif

What are the anticipated values of these variables?

I am not sure if I follow. The early return is a dirty fix I made in order to avoid tests failing, I think it should be possible to modify this class to remove the early return logic and make it work both during testing and not testing.

@nkx111
Copy link
Member

nkx111 commented Mar 28, 2022

I am not sure if I follow. The early return is a dirty fix I made in order to avoid tests failing, I think it should be possible to modify this class to remove the early return logic and make it work both during testing and not testing.

I was saying that the possible modification is like above. We can do the modification if someone specifies the REST_PATH, etc. values for testing environment.

@lobis
Copy link
Member Author

lobis commented Mar 29, 2022

I am not sure if I follow. The early return is a dirty fix I made in order to avoid tests failing, I think it should be possible to modify this class to remove the early return logic and make it work both during testing and not testing.

I was saying that the possible modification is like above. We can do the modification if someone specifies the REST_PATH, etc. values for testing environment.

But wouldn't these values such as REST_PATH be already in the environment variables?

@nkx111
Copy link
Member

nkx111 commented Mar 29, 2022

I am not sure if I follow. The early return is a dirty fix I made in order to avoid tests failing, I think it should be possible to modify this class to remove the early return logic and make it work both during testing and not testing.

I was saying that the possible modification is like above. We can do the modification if someone specifies the REST_PATH, etc. values for testing environment.

But wouldn't these values such as REST_PATH be already in the environment variables?

I guess not. REST_PATH is set only after calling thisREST.sh. Maybe the test system didn't do that.

@lobis
Copy link
Member Author

lobis commented Mar 29, 2022

I am not sure if I follow. The early return is a dirty fix I made in order to avoid tests failing, I think it should be possible to modify this class to remove the early return logic and make it work both during testing and not testing.

I was saying that the possible modification is like above. We can do the modification if someone specifies the REST_PATH, etc. values for testing environment.

But wouldn't these values such as REST_PATH be already in the environment variables?

I guess not. REST_PATH is set only after calling thisREST.sh. Maybe the test system didn't do that.

When you run ctest you should have the same env variables that when you run i.e. restManager. It will fail even after running source thisREST.sh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants