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

Fix ES unit test failures #88

Closed
skliper opened this issue Sep 30, 2019 · 8 comments
Closed

Fix ES unit test failures #88

skliper opened this issue Sep 30, 2019 · 8 comments
Labels
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

The current ES unit test has a number of failures that need to be addressed.

File attached showing current unit test report file with 27 failures.

@skliper skliper added this to the 6.5.0 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 57. Created by jphickey on 2015-05-27T11:08:46, last modified: 2019-03-05T14:57:55

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-05-29 11:16:22:

Pushed commit [changeset:ee6aa6a] for review

This does not fix the failures but it does allow the ES unit test to run to completion. When using the current version of the stub functions, it was hitting an endless loop trying to delete resources.

This can be merged in separately from any specific test fixes, as it is a prerequisite to running the test and fixing anything.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-10 13:28:19:

I've placed the CFE ES unit tests that fail into Quarantine
in the ic-2015-05-20 build ... would it be appropriate to
pick up this change to repair the test short term, or better
to wait until we have updated tests?

https://babelfish.arc.nasa.gov/bamboo/browse/CFS-CFSCFE9/latest

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-12 14:10:18:

I have done more digging into these failures and discovered that the first 3 failures (CFE_ES_Main - Normal startup) are indeed related to the start up race condition fix for trac #71.

During start up, each child thread is SUPPOSED to call "CFE_ES_WaitForStartupSync" as an indicator that they have started up. However, when running with OS stubs this does not happen because the child thread is never actually started.

Therefore the sync code ends up calling {{{CFE_PSP_Panic()}}} because the core task never started (as it was designed). In a nutshell, this prevents CFE_ES_Main from //ever// completing normally.

Unfortunately the only "solution" here (with all the limitations of the current unit test) is to remove this added check that the application actually started, and simply ignore the failure. It is ironic that we may have to make ES actually LESS robust DUE TO the the unit test architecture, something that is supposed to make code more robust.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-12 14:26:00:

I don't suppose we could augment the test stubs to do the right thing?
Or alternately, provide additional mocking so that when ES tests to
see if everything started up, it gets back a "yeah, you are fine, go
on ahead" from a mock?

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-12 14:35:25:

In this case, the code sequence between the {{{OS_TaskCreate()}}} (which is supposed to actually create the task) and the call to {{{CFE_PSP_Panic()}}} is //entirely// within CFE ES. So there is no opportunity or other stub we could hook into to "mock" it and make it look like the task actually started.

The only way to do this is to mock the counts in the CFE_ES global struct. But because it does this for //each// app, each one needs a //different// value here in order to be "correct" and pass the test.

And there is no way to hack the {{{OS_TaskCreate()}}} stub because anything done here would make it specific to the CFE_ES test and therefore break it elsewhere.

On the up-side, a better solution is possible with a better unit test library. The UT assert library does have a method to call a local "hook" as part of a stub implementation, which is what you would need in order to satisfy this sequence.

For now, and for release 6.4.2, my proposal is to take out the panic call if core tasks don't start. This is a check that was never done in prior versions, so it shouldn't hurt anything to also not do it in 6.4.2.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-23 13:42:57:

Deleting this ticket.

Further testing revealed many issues with the unit test beyond these ES failures. Ticket #103 rolls up all the UT fixes into a single branch and makes this ticket invalid.

NOTE - some of the ES unit test failures were related to the change in the startup sync, and it may be necessary to backport this to 6.4.2. If necessary a new ticket could be opened for this, it may be confusing to try to do that under this ticket.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-05 14:57:55:

Milestone renamed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant