eclipse-platform / eclipse.platform

https://eclipse.dev/eclipse/
Eclipse Public License 2.0
83 stars 113 forks source link

Replace custom stream comparison assertion with library call #903 #1510

Closed HeikoKlare closed 3 months ago

HeikoKlare commented 3 months ago

The ResourceTestUtil provides a compareContents() method for comparing the contents of two streams. This method is used in assertTrue() statements to check for equality of contents of two stream. Modern assertion frameworks already provide that functionality and, in addition, also provide proper error messages when the comparison yields differences.

This change removes the compareContents() method and replaces all calls with either direct string comparison if the indirection over a stream became unnecesary or using assertThat().hasContent() or assertThat().hasSameContentAs().

Contributes to https://github.com/eclipse-platform/eclipse.platform/issues/903

github-actions[bot] commented 3 months ago

Test Results

 1 734 files  ±0   1 734 suites  ±0   1h 26m 24s :stopwatch: + 2m 58s  3 973 tests ±0   3 951 :white_check_mark: ±0   22 :zzz: ±0  0 :x: ±0  12 516 runs  ±0  12 353 :white_check_mark: ±0  163 :zzz: ±0  0 :x: ±0 

Results for commit 01867082. ± Comparison against base commit 5f7b6e7f.

:recycle: This comment has been updated with latest results.

HannesWell commented 3 months ago

Thanks Heiko for this. For the cases where the method used to obtain the file content is not relevant, you could also use the new IFile.readString() method. Then a standard JUnit assertEquals() would be sufficient. I looked a bit into this some time ago, but didn't complete it yet. But if you want to apply this here it would be nice as well.

HeikoKlare commented 3 months ago

Thank you, Hannes! Totally makes sense to simply compare strings where possible. I found the changes in the following classes to be exchangeable with string comparisons and updated the PR accordingly:

I think there are more places we might simplify, but at several places IFile::getContents(false) is used (i.e., an out-of-sync file is not refreshed) while IFile::readString() forces a refresh. Every place needs to be investigated whether this is relevant or not, so I would propose to do this in a separate PR, if desired.

HannesWell commented 3 months ago

I think there are more places we might simplify, but at several places IFile::getContents(false) is used (i.e., an out-of-sync file is not refreshed) while IFile::readString() forces a refresh. Every place needs to be investigated whether this is relevant or not, so I would propose to do this in a separate PR, if desired.

Sounds good. :) And yes we have to be careful in this area to not change the meaning of a test. Some cases might even explicitly test the getContent() method and one should not accidentally replace that in a batch change.