-
Notifications
You must be signed in to change notification settings - Fork 8
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
Images to video generation cli command tests added (Resolves #11) #13
base: main
Are you sure you want to change the base?
Conversation
test/ffmpeg/ffmpeg_command_test.dart
Outdated
@@ -113,6 +113,42 @@ void main() { | |||
), | |||
); | |||
}); | |||
|
|||
test("converts a group of images to a video", () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that other tests in this file test technical details of the package. This test is really more of an example use-case. If we put all use cases in this file then it would be very difficult to find anything. Please put this test in a new file called test/use_cases/use_cases_test.dart
.
test/ffmpeg/ffmpeg_command_test.dart
Outdated
FfmpegInput.asset("assets/audio/audio.mp3"), | ||
], | ||
args: [ | ||
const CliArg(name: 'y'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments for why you need/want each of these arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
test/ffmpeg/ffmpeg_command_test.dart
Outdated
test("converts a group of images to a video", () { | ||
final command = FfmpegCommand.simple( | ||
inputs: [ | ||
// file%d.png represents images named like file0.png, file1.png, file2.png, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of calling things file
- how about we use names that reflect what a real file name for this purpose might look like. For example, I assume these files are screenshots?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah, they were. Will update the names.
test/ffmpeg/ffmpeg_command_test.dart
Outdated
const CliArg(name: 'c:a', value: 'aac'), | ||
const CliArg(name: 'qscale:v', value: '1'), | ||
], | ||
outputFilepath: 'assets/output/file.mp4', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you really name your output file.mp4
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update it to something more appropriate.
test/ffmpeg/ffmpeg_command_test.dart
Outdated
|
||
expect( | ||
command.toCli(), | ||
const CliCommand( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you run this command to ensure that it does what you expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah, works nicely.
@matthew-carroll Latest updates:
|
Images to video generation cli command tests added (Resolves #11)
This PR adds tests for a ffmpeg operation that takes in a group of images and an audio file and outputs a video file.