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

797 ICD tests in the backend unit tests #888

Merged
merged 104 commits into from
Nov 16, 2021
Merged

797 ICD tests in the backend unit tests #888

merged 104 commits into from
Nov 16, 2021

Conversation

markccchiang
Copy link
Contributor

@markccchiang markccchiang commented Aug 3, 2021

This PR is the trial for #797 to move some ICD tests back to the backend unit tests. I will add more testing cases according to the document).

@veggiesaurus
Copy link
Collaborator

Since the original ICD tests use some image samples with large sizes, I do not include these images in the backend repo. Users need to copy these images manually to run the test

We can automatically generate these images. Please follow the implementation @confluence used.

I also think we don't have to strictly keep the same test approach, that's not the point of this migration. If something can be tested simply as a unit test (e.g. Does Carta generate the correct file information?) then there's really no need to wrap it in then entire ICD. Just test the functionality.

Some examples that would make trivial unit tests and not require any specific ICD stuff (although that will be useful for certain tests)

  • histograms
  • file lists
  • file info
  • contours
  • region stats
  • moment maps
  • saved images

@confluence
Copy link
Collaborator

Agreed -- tests which confirm that each message has an expected and well-formed response should remain as ICD tests. Tests which check the correctness of the data should be removed from the ICD tests and rewritten as ordinary unit tests which bypass the messaging interface and call functions in the backend objects directly. Wherever possible we should use generated images.

@markccchiang markccchiang marked this pull request as ready for review September 29, 2021 05:59
src/Session.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@confluence confluence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is all fine now. I'm not 100% sure if we should be making all private members of Session protected, but I don't have a specific objection.

@pford
Copy link
Collaborator

pford commented Nov 12, 2021

I also think we don't have to strictly keep the same test approach, that's not the point of this migration. If something can be tested simply as a unit test (e.g. Does Carta generate the correct file information?) then there's really no need to wrap it in then entire ICD. Just test the functionality.

Some examples that would make trivial unit tests and not require any specific ICD stuff (although that will be useful for certain tests)

* histograms
* file lists
* file info
* contours
* region stats
* moment maps
* saved images

I am confused by this PR and the issue comments. This test framework is very high-level at the message received/response level, rather than as close as possible to the code which does the work. I thought the goal was not to duplicate the ICD tests?

@confluence
Copy link
Collaborator

My understanding is that we decided that it was a good idea to have one specific unit test which tests the Session's message handling (which is what this PR currently contains) and to add most of the converted tests (the original purpose of this PR) in stages, in multiple PRs, rather than adding them in one large PR. But perhaps one group of tests should be selected to be added to this PR, so that we can further discuss the approach here.

@markccchiang
Copy link
Contributor Author

I am confused by this PR and the issue comments. This test framework is very high-level at the message received/response level, rather than as close as possible to the code which does the work. I thought the goal was not to duplicate the ICD tests?

I agree that some tests can just be trivial unit tests. We don't need to duplicate the original ICD tests. We can simplify and migrate some ICD tests to the backend so that we can inspect in detail why some of the original ICD tests failed, like animation and moments generation. The main purpose of high-level tests is to check if the message received/response fits our expectations (in case if we want to do this kind of test for the new feature without the frontend). I think it can also be used to test the performance in the future. I will check if some of the test cases can be removed. Or just leave it as an example for the developer who wants to add more tests (@acdo2002 seems to have the interest to do it).

@acdo2002
Copy link
Contributor

I have test the ICD unit tests on the Ubuntu 20.04.
(1) After the tests, the carta_backend_test stuck there, for example:

[==========] 7 tests from 1 test suite ran. (5523 ms total)
[  PASSED  ] 7 tests.


(2) Some problem with AnimatorNavigation:

mingyi@mingyi-ASUS-ExpertCenter-D900TA-M900TA:~/carta-backend-unit/build/test$ ./carta_backend_tests --gtest_filter=IcdTest.*
Note: Google Test filter = IcdTest.*
[==========] Running 8 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 8 tests from IcdTest
[ RUN      ] IcdTest.AccessCartaDefault
[       OK ] IcdTest.AccessCartaDefault (1 ms)
[ RUN      ] IcdTest.AccessCartaKnownDefault
[       OK ] IcdTest.AccessCartaKnownDefault (0 ms)
[ RUN      ] IcdTest.AccessCartaNoClientFeature
[       OK ] IcdTest.AccessCartaNoClientFeature (0 ms)
[ RUN      ] IcdTest.AccessCartaSameIdTwice
[       OK ] IcdTest.AccessCartaSameIdTwice (0 ms)
[ RUN      ] IcdTest.AnimatorDataStream
2021-11-15 07:23:31 INFO FITSCoordinateUtil::fromFITSHeader passing empty or nonexistant spectral Coordinate axis
[       OK ] IcdTest.AnimatorDataStream (456 ms)
[ RUN      ] IcdTest.AnimatorNavigation
sh: 1: fits2idia: not found
sh: 1: fits2idia: not found
/home/mingyi/carta-backend-unit/test/TestIcd.cc:191: Failure
Expected equality of these values:
  _message_count
    Which is: 1
  3
/home/mingyi/carta-backend-unit/test/TestIcd.cc:205: Failure
Value of: open_file_ack.success()
  Actual: false
Expected: true
/home/mingyi/carta-backend-unit/test/TestIcd.cc:220: Failure
Expected equality of these values:
  _message_count
    Which is: 1
  2
/home/mingyi/carta-backend-unit/test/TestIcd.cc:242: Failure
Expected equality of these values:
  _message_count
    Which is: 1
  4
/home/mingyi/carta-backend-unit/test/TestIcd.cc:264: Failure
Expected equality of these values:
  _message_count
    Which is: 1
  4
[  FAILED  ] IcdTest.AnimatorNavigation (512 ms)
[ RUN      ] IcdTest.AnimatorPlayback
2021-11-15 07:23:32 INFO FITSCoordinateUtil::fromFITSHeader passing empty or nonexistant spectral Coordinate axis

@markccchiang
Copy link
Contributor Author

markccchiang commented Nov 15, 2021

[       OK ] IcdTest.AnimatorDataStream (456 ms)
[ RUN      ] IcdTest.AnimatorNavigation
sh: 1: fits2idia: not found
sh: 1: fits2idia: not found

Looks like you do not install the fits2idia executor?

@acdo2002
Copy link
Contributor

@markccchiang Yes, you are right, after I install fits2idia, the (2) problem disappears!!
but (1) is still there with Ubuntu 20.04....

Copy link
Collaborator

@veggiesaurus veggiesaurus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will merge in once the CI shows all tests passing

@veggiesaurus veggiesaurus merged commit 3d92b9f into dev Nov 16, 2021
@veggiesaurus veggiesaurus deleted the mark/797_ICD_tests branch November 16, 2021 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants