Use recipe:: prefix to build name

[joshk0]

minitest-chef-handler builds the full recipe name using a 'recipe::' prefix. This full name is then matched against the minitest filter shared between this cookbook and the minitest library itself. However, the minitest library has the full name including recipe::, and requires the filter to allow for that.

To permit the files to be copied at all so minitest sees them, let alone filters them properly, matches_filter? must also use the name including 'recipe::'.

An alternative to this would be to modify minitest-chef-handler, or rely on its filtering routines instead of reimplementing them, to ensure consistency. This is the quick fix that we've got in place while we figure out what the right thing to do is.

[joshk0]

This is useless without ci-reporter/ci_reporter#150 as well as vice versa if a filter is enabled. Please merge!

[dpetzel]

I'm hoping to find some time to test it this week. Can you elaborate on why its "useless" without the linked issue? I'm not sure I fully understand the relationship, but simply merging this as-is without also including whatever version of ci_reporter includes your fixe(s) won't accomplish anything correct?

Said another way, is there any value in merging this before ci_reporter cuts a build with the fixes you championed in that issue?

[b-dean]

@joshk0 there's a new ci_reporter version 1.9.3 that has the fix in it. if someone fixes the default attributes in this cookbook to have the ci_reporter gem at 1.9.3 the filters should work correctly for ci.

A workaround is to just set the attribute in your .kitchen.yml like in the platform:

platforms:
- name: ubuntu-14.04
  attributes:
    minitest:
      ci_reporter_gem_version: 1.9.3

You should already have something in there to tell it were to put the test results anyway.

But I agree with @dpetzel that this has little (if anything) to do with his issue.

[joshk0]

No, there is a strong connection. If you opt in to the correct version of the ci_reporter gem, the filters would work as advertised. But if you don't take this fix as well, the files are not even copied over, ci_reporter sees no files, and no tests are executed.

The linkage is because the same node[:minitest][:filter] is used to inform both the file copy filtering and ci_reporter's filtering.

Or perhaps I'm misunderstanding something?

[b-dean]

Sure, if you're using the CI stuff and you want to have the recipe:: on there too and have the files get copied to the correct place then you would need the new ci_reporter gem.

I didn't see in your original issue (or in the commit in this pull request) anything that was related to CI at all. As far as I could tell this was just the standard minitest reporting to STDOUT

[joshk0]

Hmm, I am confused now. I'll say my piece once more, in a different way, and then let @dpetzel figure it all out:

In the minitest handler cookbook, tests are copied according to the minitest filter, which until this change did not support the recipe:: syntax for filtering tests.

If you don't specify node[:minitest][:ci_reports], the MIniTest::Unit runner is used, which automatically prepends the recipe:: to the string that is used for test filtering.

Therefore, if you just wanted to fix these two cases without changing code, you would not use the recipe:: prefix for your test filter, and then the filtering logic would behave as designed.

However, if you DO specify node[:minitest][:ci_reports], ci_reporter is used for test filtering and execution, which until the referenced change did not prepend the recipe:: for filtering. You would then not be able to use the same filter for test copying as test filtering, without making this change.

By taking this change, the same filter expression will work for all cases, chef minitest with MiniTest::Unit and chef minitest with ci_reporter. Without this change, the filter has to be different depending on whether you specify the ci_reports option or not.

Clearer, I hope? Thanks! :)

[dpetzel]

@joshk0, this makes it much clearer (for me at least). I'm still a little fuzzy on the ci_reporter_gem_version value though. If we don't also bump that to 1.9.3 (currently its 1.9.2) wouldn't we leave the user with the old version, prior to where your fix was applied?

Apologies for all the confusion here, the filtering stuff was all added before I started maintaining this thing, and I personally don't use the functionality, so I'm just trying to get my head around the whole situation.

[joshk0]

Yes, you are right. I added a change to this pull request for that.

[b-dean]

Looks like everyone is confused because now I don't really understand what's going on either.

So @joshk0, the only place in calavera/minitest-chef-handler@7218e11603fdf6822f1c1f6d6afe55713d633f06 that I could find anything adding 'recipe::' was the the describe_recipe method: https://github.com/calavera/minitest-chef-handler/blob/7218e11603fdf6822f1c1f6d6afe55713d633f06/lib/minitest-chef-handler/spec.rb#L18

I had never tried to put 'recipe::' on any of my filters so that's probably why I never noticed it not working. They're usually something like:

attributes:
  minitest:
    filter: /.*my_cookbook(_test)?::.*/

Your original changeset for adding 'recipe::' would make it so the normal reporter (which is basically just the MiniTest::Unit class) would allow you to filter out things that had describe_recipe 'cookbook::recipe' or describe 'recipe::cookbook::recipe, it wouldn't work with the CI reporter until my fix.

So I think I get what you were trying to say now :+1:

[joshk0]

Yes, I use describe_recipe for all my tests, and the tests run in development using ci_reporter and in production using minitest.

Glad everyone is on the same page now!

[dpetzel]

Really sorry about the delay here. I've been swamped and then out of time. I really hope to get to it this week.

[dpetzel]

@joshk0 It passed all the tests (thanks!). I've merged it into master. I'm going to late it bake a bit before I cut a new release.