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

Disable failing VideoEncoder and Audioecoder tests on Windows #149

Merged
merged 10 commits into from
Jan 30, 2021

Conversation

chapulina
Copy link
Contributor

An attempt at fixing #148.

The test changes are minimal, I recommend the view ignoring whitespace changes:

  • Make test verbose
  • Create video within a scope, so we can check that it deletes all temp files on destruction
  • Output file paths in case the tests fail

The change that I hope will fix it is the use of common::removeFile instead of std::remove.

@chapulina chapulina added tests Broken or missing tests / testing infra Windows Windows support AV AV component labels Dec 29, 2020
@chapulina chapulina requested a review from caguero December 29, 2020 20:52
@chapulina chapulina requested a review from mjcarroll as a code owner December 29, 2020 20:52
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Dec 29, 2020
@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #149 (2a1ea60) into ign-common3 (f5550c0) will decrease coverage by 0.01%.
The diff coverage is 55.55%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-common3     #149      +/-   ##
===============================================
- Coverage        75.87%   75.86%   -0.02%     
===============================================
  Files               69       69              
  Lines             9758     9770      +12     
===============================================
+ Hits              7404     7412       +8     
- Misses            2354     2358       +4     
Impacted Files Coverage Δ
av/src/VideoEncoder.cc 70.64% <55.55%> (-1.60%) ⬇️
src/Filesystem.cc 80.27% <0.00%> (+2.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5550c0...ee631e8. Read the comment docs.

Signed-off-by: Louise Poubel <[email protected]>
@mjcarroll
Copy link
Contributor

@osrf-jenkins retest this please

@chapulina chapulina requested a review from JShep1 January 4, 2021 19:37
@JShep1
Copy link

JShep1 commented Jan 5, 2021

@osrf-jenkins run tests please

@JShep1
Copy link

JShep1 commented Jan 5, 2021

Seems like the test is still failing on Windows, re-running tests for now, and then will look more into it after

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

Seems like the test is still failing on Windows

Interesting, it's saying Permission denied when it tries to remove the temp file. Are the permissions of the file created by ffmpeg too strict somehow? Or is there something else happening, like the file being still in use? I don't know... Added more error messages in 93709dd to see if there are more hints.

@chapulina
Copy link
Contributor Author

Yeah this doesn't fix the issue... For some reason the temp file can't be deleted on Windows... If no one has a good idea for a fix, I suggest we comment out the test on Windows for now

@mjcarroll
Copy link
Contributor

If no one has a good idea for a fix, I suggest we comment out the test on Windows for now

No suggestion on fixing, but I would prefer to use the macro: https://github.com/ignitionrobotics/ign-cmake/blob/ign-cmake2/include/ignition/utilities/ExtraTestMacros.hh#L28

@chapulina
Copy link
Contributor Author

I would prefer to use the macro

In general I agree, but in this case, the only offending expectations were at the end of the test, so I suggest we just skip those and still run the rest of the test on Windows. See 32421c9

@chapulina chapulina changed the title Fix VideoEncoder test on Windows Disable failing VideoEncoder and Audioecoder test on Windows Jan 28, 2021
@chapulina chapulina changed the title Disable failing VideoEncoder and Audioecoder test on Windows Disable failing VideoEncoder and Audioecoder tests on Windows Jan 28, 2021
@chapulina
Copy link
Contributor Author

Also disabled #156

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a problem on Windows:

C:\Jenkins\workspace\ignition_common-ci-pr_any-windows7-amd64\ws\ign-common\av\src\AudioDecoder_TEST.cc(20): fatal error C1083: Cannot open include file: 'ignition/utilities/ExtraTestMacros.hh': No such file or directory

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

There's a problem on Windows:

Ahhh not anymore!

@chapulina chapulina merged commit f34681d into ign-common3 Jan 30, 2021
@chapulina chapulina deleted the chapulina/148 branch January 30, 2021 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AV AV component 📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice tests Broken or missing tests / testing infra Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants