[haiku-development] Re: Unit/integration test fixing strategy
- From: "Adrien Destugues" <pulkomandy@xxxxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Mon, 06 Jan 2020 21:01:17 +0000
Hey everybody!
Hi!
That lead me to the existing test suite[1], and I made it a goal to get
that into good shape before proceeding with my planned changes.
Thanks for working on this! A working test suite would indeed make our life
better and easier as Haiku developers.
In cases where there is divergence, I've found that the tests were
designed to work in BeOS R5 and the BeOS behavior is defined in the
tests (although I'm double checking each case).
The tests are old, at the time the intent was (I guess, I hadn't joined the
project yet back then) to really explore how BeOS behaves in every detail.
But our goals have shifted a little since then.
Case 1: Regression that causes tests to fail
--------------------------------------------
hrev53662[3] fixed a regression in BString that caused a test to fail.
This was a clear regression that caused the test to fail, so changing
BString, and not the tests, was the right thing to do.
No questions asked here.
Another more silly example is in this change[4] from one of my
branches, where this code path in BClipboard traps in the debugger,
without the call to debugger() the correct, BeOS R5 compatible error
code is returned.
debugger() calls are a way to warn developers when they do something
strange with the API that is an obvious error on their side. It is
possible to call disable_debugger() at the start of an application to
make them innefective. Maybe the test in question could do this, and
so could apps built in release mode if they wish to.
Another example is here in BArchivable[8], where the BeOS R5 code
allows instantiate_object without providing a signature (mime-type)
value. In R5 it will just handle the missing signature field in the
BMessage and will search all loaded images for the specified type. This
seems like a clear regression where the implementation of
instantiate_object(BMessage*) should be changed and not the tests.
This should be fixed if possible, and the behavior documented either way
in the Haiku book (either as normal description of the function, or as
a warning indicating a difference with BeOS)
Case 2: Divergence from BeOS that is desired
--------------------------------------------
hrev53681[2] fixed the BMemoryIO tests. There was one regression, and
it was related to the fact that BMemoryIO::WriteAt(off_t, const void*,
size_t)
In this case, one of the BMemoryIO tests was passing in BeOS R5, but
not in Haiku, because the Haiku code was better and detects a potential
memory corruption bug. It Haiku's BMemoryIO prevents the use of some
invalid input that has some undefined side effect that is likely not
going to be desired.
In this case I think it's clear that the Haiku behavior is desired and
the test was updated appropriately.
Yes. We can introduce quirks for BeOS apps if we find out they are not
working because of such changes and we decide it is worth it (usually
recovering the sourcecode for the app and fixing it, or replacing it
with something else is easier, except for the few largest apps).
Case 3: Haiku ABI has diverged from BeOS R5
-------------------------------------------
I haven't run into a case of this yet, but I haven't doen analysis on
all of the failed tests yet.
My assumption is that any ABI break should be resolved by updating the
Haiku ABI to match BeOS R5 under all circumstances.
Yes, with the nuance that we can introduce symbol versionning or other
tricks where desirable. For example, the get_system_info ABI was modified
to allow more than 8 CPUs to be reported, but compatibility is provided
for existing applications.
Case 4: Divergence from BeOS that is of unclear value
-----------------------------------------------------
These are all over the place, and this is where I need some guidance.
I have branchs which fixe most of the tests for BArchivable and
BRoster, and while there are some regressions, most of the changes are
related to differences in error codes.
One example of an error-code mismatch is here in instantiate_object[5].
In Haiku, the B_LAUNCH_FAILED_APP_NOT_FOUND returned from
BRoster::FindApp() is bubbled up to the caller of instantiate_object(),
where in BeOS R5 the error returned is B_BAD_VALUE. It is not clear to
me whether it is best to attempt to match all error codes in BeOS, or
if it's best to leave this unchanged to avoid breaking Haiku userspace
that has come to expect this error.
A more specific error code seems better to me. I see no point being this
close to BeOS. Our goal is to be API compatible, and the API just says
"this returns a status_t", so well behaved apps should be ready to handle
any error code.
This change[6] is a trickier example, where is given
instantiate_object() a valid signature matching an add-on, but once the
add-on is loaded, the type name is not found. In Haiku, the image_id
parameter is set to an error value, while in BeOS, the image_id
parameter is the ID of the add-on that was loaded by
instantiate_object(). This behavior difference may be unimportant, and
it isn't really documented, but it is different. One may argue that the
BeOS behavior is desired since you may want the option to unload the
add-on if the type you're trying to load was not found, but this
behavior is not documented[7].
Doesn't the Haiku version unload the add-on by itself? If so, we introduce
a leak here, and we shoudl fix that. It doesn't really matter if we do
this by setting image_id to an error value and unloading the image,
or by replicating BeOS behavior. I would tend to pick the first option,
as it seems unsafe to rely on the app for unloading the add-on here.
So: let's just go for what makes more sense for this API.
In this case, I think the algorithm should be:
* If the behavior difference is purely a difference in error codes, and
those error codes are not documented in the Haiku API docs, then just
change Haiku to match BeOS.
* Else if the behavior difference *is* documented in the Haiku API
docs, then update the tests and the docs, noting that the behavior
has diverged from R5.
* Else, deside on a case-by-case basis which side to prefer, preferring
to have as little impact on Haiku userspace as possible.
I think this is always going to be case-by-case. There are many factors to
take into account, such as:
- Does this break BeOS apps in a realistic scenario?
- Can the broken BeOS app be fixed?
- Does it break newly built apps for Haiku?
- Does the behavior make sense?
Documentation can be fixed, and can be expanded to point out differences
with BeOS where needed.
If we find out a particular change breaks a BeOS app, we can always go back
and document the test case to link it with the app that needs the exact
behavior.
Then, we know the things that are really important to keep.
--
Adrien.
Other related posts:
