Add crystal spec --list-tags

[baseballlover723]

Fixes #13615.

Draft since this is just a proof of concept at the moment. Currently this works by specifying a new option to crystal spec --list-tags. When this option is specified, the tests won't be run at the example level and instead it will iterate over all of it's parent contexts, incrementing a counter for each tag associated with the example. Then at the end of the test run it will output the counts of each spec tag.

I'm not 100% convinced this is the best way to go about this, but this does have a bunch of nice things. For instance, this works out of the box with stuff like specifying a dir to run on, focus or even --tag tag. The downside is that the specs still have to compile (not sure if this is even possible with a failing compile. My intuition says that as long as the spec structure is valid it should be possible to calculate. But I don't know enough about the compiler to say if this is possible or feasible with the current compiler.), and the counting is done at runtime, so it runs GroupExample before and after blocks (could be a potential time sink. For example setting up a DB. I'm not 100% sure we can omit these blocks, since I think that it's possible for before and after blocks to affect if a test is run or not, which is relevant to finding the spec tags. I might be thinking of RSpec or another language or something though). It also displays the after normal test output , which doesn't really make sense for this I think.

Finished in 504 microseconds
0 examples, 0 failures, 0 errors, 0 pending

An example spec file I've been using for testing

describe "My test spec" do
  it "untagged #1" do
    puts "\nrunning untagged #1"
  end
  it "untagged #2" do
    puts "\nrunning untagged #2"
  end
  it "untagged #3" do
    puts "\nrunning untagged #3"
  end

  it "slow #1", tags: "slow" do
    puts "\nrunning slow #1"
  end
  it "slow #2", tags: "slow" do
    puts "\nrunning slow #2"
  end

  it "untagged #4" do
    puts "\nrunning untagged #4"
  end

  it "flakey #1", tags: "flakey" do
    puts "\nrunning flakey #1"
  end
  it "flakey #2, slow #3", tags: ["flakey", "slow"] do
    puts "\nrunning flakey #2, slow #3"
  end

  it "untagged #5" do
    puts "\nrunning untagged #5"
  end

  pending "untagged #6"

  pending "untagged #7" do
    puts "\nrunning untagged #7"
  end

  describe "describe specs", tags: "describe" do
    it "describe #1" do
      puts "\nrunning describe #1"
    end
    it "describe #2" do
      puts "\nrunning describe #2"
    end
    it "describe #3, slow #4", tags: "slow" do
      puts "\nrunning describe #3, slow #4"
    end
    it "describe #4, flakey #3", tags: "flakey" do
      puts "\nrunning describe #4, flakey #3"
    end
  end
end

which ./bin/crystal spec #{path} --list-tags outputs

Using compiled compiler at .build/crystal
untagged: 5
describe: 4
    slow: 4
  flakey: 3

At this stage I would like some feedback on the general design of this (I could see this being under crystal tool instead, though I think thats more work). Right now it feels pretty hackish.

I think this is ready for review now. I'm going to leave off handling a --verbose flag, since I think it's easier to just specify a path if you want more specific information and the printing seems a bit complex then I want to bite off at the current moment. Currently there aren't any tests, I'm happy to add some tests, but I'm not sure what the best way to test this is.

[beta-ziliani]

Mmm... tough one. Ideally, it should compile but no codegen and no run (in any form) the specs. So from that point of view, probably crystal tool (or at least, its infrastructure) is more appropriate. But for that the tags should be compile-time info, and not runtime, which is a bigger change.

[straight-shoota]

I'm not convinced about piggy bagging on the formatter mechanism for this. It's a fundamentally different thing and completely unrelated to example runs and result formatting.

I think this command should hook right into the at_exit handler in Spec.run and traverse the context tree to collect tags.

[baseballlover723]

I'm not convinced about piggy bagging on the formater mechanism for this. It's a fundamentally different thing and completely unrelated to example runs and result formatting.

I think this command should hook right into the at_exit handler in Spec.run and traverse the context tree to collect tags.

I'm not 100% sure what you mean. The formatter stuff wasn't essential to anything and could easily be removed. The bulk of the change was in the example running part to tally up the tags instead of running the example.

Did you actually want me to make the code branch at the at_exit level? It seems like a lot more work imo, since I'd have to duplicate and then modify the whole run chain. If your objection is just the formatter, I think it would be easier to just to revert that part, while still piggybacking on spec running functionality (essentially just the same as running the specs, but with it tallying tags instead of running any examples).

Let me know what you think, I just want to clear up this up, because it seems like you might of misunderstood how involved the formatter was. I've pushed a commit removing all of the formatter stuff for now and updated the output in the description.

[straight-shoota]

That looks much better, thanks 🙇 I'll take a deeper look later.

[baseballlover723]

One thing to note now, is that running with --verbose results in messed up formatting, since it runs the example start formatter hook, but never the end (this is to avoid printing the dots, since it doesn't actually run the examples). Probably should solved by just not running the before example hooks if we're still gonna piggyback on the normal spec running. Another option could be to disable --verbose and --list-tags together or just ignore it (this doesn't feel all that good though, for something built into the language, imo it should be more graceful handling this).

[straight-shoota]

With --list-tags no hook should execute. It's supposed to be an entirely informational command.
The only code that needs to run is the top-level code that defines the examples and groups.

[straight-shoota]

I have taken a dip to move the implementation entirely outside the run code path. It's not that much actually and clearly separates concerns by not nesting different functions into each other.
This should show clearly what I have intended:

--- c/src/spec/dsl.cr
+++ w/src/spec/dsl.cr
@@ -206,10 +206,11 @@ module Spec
     @@start_time = Time.monotonic

     at_exit do
-      log_setup
-      maybe_randomize
-      run_filters
-      root_context.run
+      if Spec.list_tags?
+        execute_list_tags
+      else
+        execute_examples
+      end
     rescue ex
       STDERR.print "Unhandled exception: "
       ex.inspect_with_backtrace(STDERR)
@@ -220,6 +221,53 @@ module Spec
     end
   end

+  # :nodoc:
+  def self.execute_examples
+    log_setup
+    maybe_randomize
+    run_filters
+    root_context.run
+  end
+
+  # :nodoc:
+  def self.execute_list_tags
+    run_filters
+
+    index = Hash(String, Int32).new(0)
+    collect_tags(index, root_context, Set(String).new)
+
+    longest_name = index.keys.max_of(&.size)
+    index.to_a.sort_by! { |k, v| -v }.each do |tag_name, count|
+      puts "#{tag_name.rjust(longest_name)}: #{count}"
+    end
+
+    exit
+  end
+
+  # :nodoc:
+  private def self.collect_tags(index, context : Context, tags)
+    if context.responds_to?(:tags) && (context_tags = context.tags)
+      tags += context_tags
+    end
+    context.children.each do |child|
+      collect_tags(index, child, tags)
+    end
+  end
+
+  # :nodoc:
+  private def self.collect_tags(index, example : Example, tags)
+    if example_tags = example.tags
+      tags += example_tags
+    end
+    if tags.empty?
+      index.update("untagged") { |count| count + 1 }
+    else
+      tags.each do |tag|
+        index.update(tag) { |count| count + 1 }
+      end
+    end
+  end
+

This may need some polishing and I haven't tested it extensively. But it should be a solid basis.
Feel free to incorporate this diff into this PR if you want.

[baseballlover723]

I've updated this PR with @straight-shoota's changes as the core. I've done some polishing, but let me know if theres anything else you want changed. I also added a separate section for pending examples, since I think it could be useful to differentiate between regular and pending examples. I've updated the description with the latest output.

I'll take a look at making some tests for this next week. I'm thinking integration style tests with a spec file and running in a sub process should be used at some level, but I'll do some digging next week. Let me know if you have any opinions off the top of your head, otherwise I'll just figure something out next week.

[baseballlover723]

I've added a spec file to this PR. It seemed pretty awkward to do something other then compiling and running a spec file and then asserting the output, so that's all I did. The test specs were derived from the spec I was using for manual testing with this, which I think is pretty comprehensive, but let me know if there's anything else you want me to add or explicitly test.

This only tests the filtering by tag, since that's easily tested with another runtime flag. This also support filtering by file / line, but I think that's alright to skip testing for this, since that has its own spec file.

[baseballlover723]

@straight-shoota could you review this again? Thanks!

[baseballlover723]

I've updated this PR to not distinct between pending and non pending examples as per #13615 (comment)

[baseballlover723]

@HertzDevil can you review this? I think it would be good to get this in for 1.11.0 with #13804.