CARTAvis / carta-backend

Source code repository for the backend component of CARTA, a new visualization tool designed for the ALMA, the VLA and the SKA pathfinders.
https://cartavis.github.io/
GNU General Public License v3.0
22 stars 11 forks source link

Migrate possible ICD test back to backend unit test #797

Open kswang1029 opened 3 years ago

kswang1029 commented 3 years ago

This is a step of making the automated test robust and informative. Detailed list of which ICD tests to be migrated is TBD (work-in-progress list here)

todo list based on the C column of the above spreadsheet (each should be a separate PR):

veggiesaurus commented 3 years ago

My feeling on the migration is that we should rely on unit tests for:

If we use external data for the unit tests (e.g. images, tables, region files), they should either be small or auto-generated, so that they can be included in the repo.

veggiesaurus commented 3 years ago

@confluence and I have had a few discussions on how to actually implement some of these tests. One of the difficulties is mocking up some of the classes, so that we can test things in isolation.

confluence commented 3 years ago

I think we should review some existing libraries that may be able to do this for us, to avoid reinventing the wheel.

veggiesaurus commented 3 years ago

unless I'm mistaken, we can create a Frame without mocking anything else. That should allow us to do a large number of these tests with limited mocking

veggiesaurus commented 3 years ago

@markccchiang we can ask @jolopezl to implement some tests as well. We should not try to implement all the tests in a single PR, but rather chip away at them bit-by-bit.

I suggest we leave things that involve mocking up a Session class until the end.

kswang1029 commented 3 years ago

@markccchiang @veggiesaurus I modified the issue description a bit with a list of sub-tasks (not in particular order). This should help to avoid having a giant PR in the end. Does it make sense?

markccchiang commented 3 years ago

@markccchiang @veggiesaurus I modified the issue description a bit with a list of sub-tasks (not in particular order). This should help to avoid having a giant PR in the end. Does it make sense?

Thank you! That is helpful for me.

kswang1029 commented 3 years ago

@markccchiang @veggiesaurus I modified the issue description a bit with a list of sub-tasks (not in particular order). This should help to avoid having a giant PR in the end. Does it make sense?

Thank you! That is helpful for me.

@markccchiang please let me know if you have any questions per test migration. I guess there might be some that can be simplified 🤔

markccchiang commented 2 years ago

I think creating a TestSession class that inherits the Session class is a way to do (relatively) low-level unit tests. Now all Session functions are either public or protected, so the TestSession can call these functions to do the test for some cases. If we can not only use Frame or the other low-level classes for testing.

veggiesaurus commented 2 years ago

@markccchiang if you'd prefer to make separate issues for the remaining tests and story point them, that would be fine. I know it can get a bit frustrating endlessly working on one giant issue :smile:

Kechil commented 2 years ago

@markccchiang please go ahead and make separate issues for the remaining tests, and create an Epic for them to go under. Thank you. We can then close this giant task.

kswang1029 commented 2 years ago

@markccchiang @acdo2002 @ajm-asiaa could you compile a list about

  1. what ICD tests have been migrated to CPP unit tests?
  2. what ICD tests are still activated in CI?

Once we have the two lists, we can close this issue and I will make separate issues to continue the migration process.

ajm-ska commented 2 years ago

@kswang1029 Good idea. Here is the current list if ICD tests that are currently active in Jenkinsfile-full:

ACCESS_WEBSOCKET.test.ts GET_FILELIST_ROOTPATH_CONCURRENT.test.ts FILEINFO_FITS_MULTIHDU.test.ts FILEINFO_EXCEPTIONS.test.ts

ANIMATOR_DATA_STREAM.test.ts ANIMATOR_NAVIGATION.test.ts ANIMATOR_CONTOUR_MATCH.test.ts ANIMATOR_CONTOUR.test.ts

REGION_STATISTICS_RECTANGLE.test.ts REGION_STATISTICS_ELLIPSE.test.ts REGION_STATISTICS_POLYGON.test.ts

CASA_REGION_INFO.test.ts CASA_REGION_IMPORT_INTERNAL.test.ts CASA_REGION_IMPORT_EXPORT.test.ts CASA_REGION_IMPORT_EXCEPTION.test.ts CASA_REGION_EXPORT.test.ts DS9_REGION_EXPORT.test.ts DS9_REGION_IMPORT_EXCEPTION.test.ts DS9_REGION_IMPORT_EXPORT.test.ts

PER_CUBE_HISTOGRAM.test.ts PER_CUBE_HISTOGRAM_HDF5.test.ts PER_CUBE_HISTOGRAM_CANCELLATION.test.ts

PV_GENERATOR_CANCEL.test.ts PV_GENERATOR_CASA.test.ts PV_GENERATOR_FITS.test.ts PV_GENERATOR_HDF5_COMPARED_FITS.test.ts PV_GENERATOR_MATCH_SPATIAL.test.ts PV_GENERATOR_NaN.test.ts PV_GENERATOR_WIDE.test.ts

CHECK_RASTER_TILE_DATA.test.ts TILE_DATA_REQUEST.test.ts

CATALOG_GENERAL.test.ts

MOMENTS_GENERATOR_CASA.test.ts MOMENTS_GENERATOR_EXCEPTION.test.ts MOMENTS_GENERATOR_SAVE.test.ts MOMENTS_GENERATOR_CANCEL.test.ts MOMENTS_GENERATOR_FITS.test.ts MOMENTS_GENERATOR_HDF5.test.ts

RESUME_CATALOG.test.ts RESUME_CONTOUR.test.ts RESUME_IMAGE.test.ts RESUME_REGION.test.ts

MATCH_SPATIAL.test.ts MATCH_STATS.test.ts

CLOSE_FILE_SINGLE.test.ts CLOSE_FILE_ANIMATION.test.ts CLOSE_FILE_ERROR.test.ts CLOSE_FILE_SPECTRAL_PROFILE.test.ts CLOSE_FILE_TILE.test.ts