Bug 1869719

I'll take this for a review.

Looks fine, I just have 2 nits:

There is some documentation in `doc` directory in the image_processing project that I think should be included since it looks like it has more content than the generated docs.
Maybe transform it into some kind of manpage or include it in the *-doc subpackage?

And just a triviality:
This `%{gem_instdir}/image_processing.gemspec` could use a macro so it becomes `%{gem_instdir}/%{gem_name}.gemspec`.

* Unversioned `%gemspec_remove`
  - I don't like the usage of `%gemspec_remove` without specifying versions.
    If I am not mistaken, when the dependency is removed upstream, then it
    would pass without noticing. It is not really big deal, but it is nice
    to know there is something to remove if it is not needed anymore.

* `skip` vs `return`
  - It probably won't make any real difference, but wouldn't it be better to
    use `sed -i '/Vips::/ i \  return' test/test_helper.rb` instead of
    `sed -i '/Vips::/ i \  skip' test/test_helper.rb`?

(In reply to Jaroslav Prokop from comment #2)
> Looks fine

You have probably forgot fedora-review+?

Yeah right,(In reply to Vít Ondruch from comment #4)
> You have probably forgot fedora-review+?

Right, package approved!

Thank you both for your reviews!

I've added %bcond_with to disable the tests once ruby-vips is in Fedora (the test suite suceeds without skips).


> There is some documentation in `doc` directory in the image_processing project that I think should be included since it looks like it has more content than the generated docs.

Oh, you mean the upstream `doc` directory. Well, feel free to create a PR to add documentation into the gem (either generated, like other gems, or just the *.md files; that's up to the upstream).

> This `%{gem_instdir}/image_processing.gemspec` could use a macro so it becomes `%{gem_instdir}/%{gem_name}.gemspec`.

Sure, can be both ways (updated).

_ _ _ _


> * `skip` vs `return`

To me, the benefit of `skip` is that I see "SSSSSSSSS" in the test output, so I know the tests are skipped actually. `return` would just hide the issue, right?

> * Unversioned `%gemspec_remove`

It fails actually: https://gist.github.com/pvalena/aeda48c6fa786f2f3d41f996c3a6c11a

An like I wrote previously, I'll not be running %gemspec_remove anyway, once ruby-vips is in Fedora.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-image_processing

(In reply to Pavel Valena from comment #6)
> > * `skip` vs `return`
> 
> To me, the benefit of `skip` is that I see "SSSSSSSSS" in the test output,
> so I know the tests are skipped actually. `return` would just hide the
> issue, right?

That is nice of course, but if the test case contains more asserts, the `skip` won't execute the following asserts, while the `return` does. So what are pros/cons? :)

""" the `skip` won't execute the following asserts """

Are you sure about that? I've always thought it's the other way around:

```
# SKIP:
DEBUG: # Running:
DEBUG: SSSSSSSSSSSSSSS..S.SSSSSSSS.....S..S...SSS..SSS...S.SS..S..S..................S.S...S.S.
DEBUG: Finished in 3.638647s, 24.1848 runs/s, 20.3372 assertions/s.
DEBUG: 88 runs, 74 assertions, 0 failures, 0 errors, 41 skips

# RETURN:
DEBUG: + ruby -Ilib:test -e 'Dir.glob "./test/**/*_test.rb", &method(:require)'
DEBUG: Run options: --seed 39129
DEBUG: # Running:
DEBUG: ........................................................................................
DEBUG: Finished in 4.193257s, 20.9861 runs/s, 19.5552 assertions/s.
DEBUG: 88 runs, 82 assertions, 0 failures, 0 errors, 0 skips
```

And with the same test-set, but vips enabled:

```
DEBUG: Run options: --seed 32767
DEBUG: # Running:
DEBUG: ........................................................................................
DEBUG: Finished in 5.651056s, 15.5723 runs/s, 26.5437 assertions/s.
DEBUG: 88 runs, 150 assertions, 0 failures, 0 errors, 0 skips
```

aaand ... now I'm even more confused. So the return has (8) more assertions, and `skip` does not count show all assertions skipped (return hides those, obviously)?

(In reply to Pavel Valena from comment #9)
> """ the `skip` won't execute the following asserts """

The `skip` or `return` are placed into custom assertion if I am not mistaken. `skip` raises exception, so effectively fails the whole test case, while `return` just exits the specific assertion and let the following assert do its job.