Add Stream tests

[maciejbocianski]

Description

Add Stream class tests

Not ready for review yet!!!

Status

IN DEVELOPMENT

Migrations

NO

[maciejbocianski]

Guys @bulislaw @0xc0170
I have some concerns about seek function from platform/FileHandle.h
Its definition is inconsistent with C/C++ standard which says: ​
"return 0​ upon success, nonzero value otherwise"
Whereas our implementation says:
"@return The new offset of the file, negative error code on failure"

[0xc0170]

@geky will know more, from what I recall this is similar to what is defined in posix (http://man7.org/linux/man-pages/man2/lseek.2.html) ?

[geky]

Sorry @maciejbocianski, where were you seeing the "return 0" part?

FileHandle::seek should be the same as the standard lseek function:
http://pubs.opengroup.org/onlinepubs/009695399/functions/lseek.html

Upon successful completion, the resulting offset, as measured in bytes from the beginning of the file, shall be returned. Otherwise, (off_t)-1 shall be returned, errno shall be set to indicate the error, and the file offset shall remain unchanged.

[maciejbocianski]

If FileHandle::seek is assumed to be POSIX compliant then it's fine.

My concern was rather about passing result of FileHandle::seek via _lseek(mbed_retarget.cpp) to stdlib fseek (fseek return ​0​ upon success). But if _lseek is also assumed to be POSIX compliant then it is fine, and likely fseek translate POSIX ret val to C ret val (POSIX: the resulting offset -> C/C++: ​0​ upon success)

[geky]

Ah I see, the difference between fseek and lseek is a good thing to point out.

The retargeting layer for each toolchain expects an lseek like function. (ARMCC's _sys_seek says "The result is non-negative if no error occurs").

After the retargeting, it's up to the standard library to map the return values correctly to fseek. Have you ran into an instance where the return value is getting all the way to fseek?

[kjbracey]

I can see why you'd logically expect Stream::seek to be fseek, like all the other "pass-to-stdio" methods. But Stream is designed for device-like things, not anything seekable, so it doesn't provide that - you're just seeing the underlying FileHandle::seek (ie lseek). Bit of a name collision, and an example of the confusion Stream causes with its multi-layer API.

Overriding Stream's seek seems a bit like starting to push it beyond what it's intended to do, which is provide a FileHandle for simple unseekable _getc/_putc character devices. If the seek is just to let the test read what you wrote, I'd be inclined to use separate read+write pointers or a CircularBuffer, to make it a simple buffered loopback device, no seek required.

(I'm currently fighting to get Stream deprecated over here - #5655 - so that people just use FILE and FileHandle. Debates ongoing).

[maciejbocianski]

After the retargeting, it's up to the standard library to map the return values correctly to fseek. Have you ran into an instance where the return value is getting all the way to fseek?

I have done tests with all compilers and everything looks fine.
Now seeing whole picture, found my concerns baseless

[maciejbocianski]

@bulislaw do we really need tests for Stream ?
Since likely it will be deprecated soon #5655

[kjbracey]

Since likely it will be deprecated soon #5655

That's still subject to discussion - not sure there's any consensus behind it, so I wouldn't go as far as "likely" - more just "possible".

Still, you could maybe test the C retargeting systems by implementing a FileHandle directly instead of using Stream, open it with fdopen, and test C library FILE functions on it. I think that system needs testing, and it doesn't need to be tested via Stream.

Your last 3 tests would map straight to FileHandle version - the first 2 name tests could be retained if you chose to implement a FileLike rather than just a FileHandle.

If it becomes a FileHandle/FileLike test, it's still really mainly testing mbed_retarget.cpp like the Stream version was. (Note that with the FileLike the point is that you should be able to just fopen the filename directly - FileBase::lookup is the lower-level mechanism to enable that. I'd test the fopen).

[maciejbocianski]

@bulislaw what do you think about rebranding this test to FileLike and do low-level test of FileLike(FileHandle) testing as @kjbracey-arm suggested, and for now postpone Stream testing?

[bulislaw]

I would say lets not throw away the tests we just created, deprecated or not it'll be with us for a while. I would add separate tests as mentioned by Kevin.

[0xc0170]

@maciejbocianski Is this obsolete? FileHandle tests were integreated, please let us know (close PR if it can be)

[0xc0170]

Reopen if it's still relevant