Fix #36: Created TBL checkout test#55
Conversation
ddstewar
left a comment
ddstewar
left a comment
There was a problem hiding this comment.
You have made this cFE app dependent on 4 optional cFS applications. DS, HK, and TO_Lab, and MD. This is unacceptable. The cFE checkout tests can not be dependent on any application outside of the cFE, except the Sample_app which is there as a demo app for people to see how to start development of their own cFS application. See comment below about that.
You send the same 2 commands 3 times : CFE_TBL_CMD_LOAD and CFE_TBL_CMD_VALIDATE.
This is not following what these checkout tests need to do.
CFE_TBL_CMD_ACTIVATE is sent 2 times when once should be all that is done.
This needs to be reduced to 2 times for the load command just so it can be aborted the second time. The others only need once.
I'm pretty frustrated with this, since every other line is not following the spirit of the issue essentially. This test makes it look like you are just taking all the commands in the other existing tests and just putting them in here without planning how they will be run. This is not a refactoring of those tests. These need to be strategic and lean tests. these need to run fast. Repeating the same command and putting a second get-and-check that resets the cmd_count flies in the face of that. Everything looks just thrown in here, like AI wrote this. I know you can do better. Consider what the test is supposed to do, consider all of the command the app has, and plan the test out accordingly.
| test_table_name = "DS.FILE_TBL" | ||
| test_table_filename = "/cf/ds_file_tbl.tbl" |
There was a problem hiding this comment.
The TBL test can not be dependent on an app outside of the cFE. This needs changed to either a table used by the TBL app itself or one of the cFE app tables.
| cmd_count = tlm(f"<%= target_name %> CFE_TBL_HK COMMAND_COUNTER") | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_VALIDATE with ACTIVE_TABLE_FLAG INACTIVE, TABLE_NAME '{test_table_name}'") | ||
| wait_check(f"<%= target_name %> CFE_TBL_HK COMMAND_COUNTER >= {cmd_count + 1}", 100) |
There was a problem hiding this comment.
This block is overkill.
This is resetting the cmd_count variable. The tlm() call is not needed here and the wait_check() should be changed to just a simple wait() for however long is safe to move on, which is usually less time than it takes for a tlm packet to be seen.
This needs to be changed to just the validate command with a wait() after it, if any wait is needed at all.
There was a problem hiding this comment.
Good catch out resetting cmd_count - I missed that.
wait_check() should be changed to just a simple wait() for however long is safe to move on, which is usually less time than it takes for a tlm packet to be seen.
I had experimented with watching it change as it was running, and it took a significant amount of time - I think I remember it taking more than one telemetry cycle. Is there an exact definition of how long it takes? It seems like it might vary. I looked for a telemetry point that defines when validation is done, and that one made the most sense. I don't think spending a few more seconds waiting for telemetry ONE extra time is a big deal.
There was a problem hiding this comment.
That's fine if you have a specific point to watch and makes sense.
There are 2 buffers though right? So I would try sending the validate and forgetting it and move with the rest of the commands, then send the activate later in the command list for that particular table.
| test_table_name = "HK.CopyTable" | ||
| test_table_filename = "/cf/hk_cpy_tbl.tbl" | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_LOAD with LOAD_FILENAME '{test_table_filename}'") | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_VALIDATE with ACTIVE_TABLE_FLAG INACTIVE, TABLE_NAME '{test_table_name}'") |
There was a problem hiding this comment.
These 4 lines are not needed. This has already executed the load and activate commands
There was a problem hiding this comment.
Okay. It uses a different table, but I see now that nothing after it depends on this load, for the purposes of acceptance testing.
| test_table_name = "TO_LAB.Subscriptions" | ||
| test_table_filename = "/cf/to_lab_sub_bad.tbl" |
There was a problem hiding this comment.
No reason to change the table being used. Just load the same table again.
There was a problem hiding this comment.
If trying the send and forget I mentioned above, this would need to be a different table.
| test_table_name = "TO_LAB.Subscriptions" | ||
| test_table_filename = "/cf/to_lab_sub_bad.tbl" | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_LOAD with LOAD_FILENAME '{test_table_filename}'") | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_VALIDATE with ACTIVE_TABLE_FLAG INACTIVE, TABLE_NAME '{test_table_name}'") |
There was a problem hiding this comment.
The table does not need validated to have the abort be accepted. I have tested this via sending the command myself. Remove this line.
There was a problem hiding this comment.
Okay. It was included in the original test written by Ariel and Keegan. I trusted that they knew what they were doing there, and "load/validate/activate" is the usual sequence, so it seemed needed. Ask them why they thought it was needed.
There was a problem hiding this comment.
I don't care about the original test. this is not just a refactor of the basic test that we threw together to get a test out the door. That will be refactored later. this test has it's own goals to accomplish and should only loosely be based on the other one. I don't see why who wrote that has anything to do with how this test works. You should have sent the commands yourself to see what was needed at a bare minimum.
| cmd(f"<%= target_name %> CFE_TBL_CMD_VALIDATE with ACTIVE_TABLE_FLAG INACTIVE, TABLE_NAME '{test_table_name}'") | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_ABORT_LOAD with TABLE_NAME '{test_table_name}'") | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_DUMP_REGISTRY with DUMP_FILENAME 'dump_registry.dat'") | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_SEND_REGISTRY with TABLE_NAME 'MD.DWELL_TABLE4'") |
There was a problem hiding this comment.
MD is not a cFE app. The send_registry command needs to use a table from the cFE.
| cmd(f"<%= target_name %> CFE_ES_CMD_STOP_APP with APPLICATION 'MD'") | ||
| wait(10) | ||
| cmd(f"<%= target_name %> CFE_TBL_CMD_DELETE_CDS with TABLE_NAME 'MD.DWELL_TABLE4'") |
There was a problem hiding this comment.
This set of commands is stopping the MD app. The MD app can now no longer be tested.
If the delete CDS command needs to stop an app to successfully complete then it can use the sample_app. The file header here should state that the test is dependent on the Sample_app, as it is an app designed to use in these situations. The app should be restarted after this but you do not need to verify that it completed (not the point of this test.
There was a problem hiding this comment.
Good catch. There was originally a command at the end to re-start the MD app. I considered keeping it, but then I remembered you wrote in the ticket "Preservation of environment is not a concern other than not to ruin the system for later apps. Files created can be left on the system. No need to clean-up after the test." So I deleted it to keep it simple, since simple is what you're emphasizing, but I forgot that having MD stopped could "ruin the system for later app [tests]".
No, I did not. Ariel and Keegan did, in the original TBL test. And Mike Yang and Keegan did in the original SB test. And crossover has been a common thing in cFE/cFS tests when needed, and you previously never said anything about avoiding it here. It was not in the ticket description. Why should crossover be allowed in those test but not these? Do we need to open tickets to refactor their SB and TBL tests? See my earlier comment on the SB checkout test. "This is unacceptable / Everything looks just thrown in here, like AI wrote this / just putting them in here without planning how they will be run" No. Please take a step back. You're being very disrespectful, insulting, and aggressive. Not appropriate for the workplace. This is not an online forum debate with strangers on the internet. If you wanted clarification on these things, it would have been better to set up a call or an office visit. I evaluated each command before deciding whether or not it seemed needed, and whether or not the lines in-between are needed for those commands to work. Again, you never said anything about not being allowed to use CFS apps - and they were used in the functional tests (written by Mike Yang, Keegan, and Ariel) - why should they be allowed there but not here? |
Ariel and Keegan did not write this test. You did, so the statement stands. These comments only pertain to this test in isolation. not all of the tests for TBL.
I'm very frustrated at the state of this test. You may think I'm being all being those things but I simply stating my view of this test. I had to comment on every other line because of the way it was written. I made the issue text as detailed as possible because you wanted it that way, to avoid exactly this happening, yet it still happened. From the issue itself:
So yes, I did previously say in the issue that things like this should be limited to cFE or the app itself. I also talked to you in person about these tests and what their goal was, when they would be run and even answered your "what if's" that you had about them. The other tests are not functional tests, it does not matter who wrote them so I don't know why you keep referencing that. Those tests will have issues written to refactor them into functional tests. They were rushed out and there was very little that needed verified at the time. Now we are getting to the part where a higher scrutiny will be on the tests and what they test. We need higher fidelity tests all around. These checkout tests are the first step to running in the Git pipeline to check that nothing is broke before a PR is merged. The future functional tests will be run at a slower pace. perhaps once a week. There is no tie to the other tests other than you can use those a reference of what commands there are and how to use the parameters. Things will have to change for these checkout tests. |
3 participants
No description provided.