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

Expand test directory command resolution with requires #3335

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Mar 20, 2025

Motivation

Our implementation of running all files in a directory was incorrect. You cannot use wildcards as part of the path because those are only expanded in the shell, not in a background command.

Implementation

To my surprise, running ruby -Itest test/file1_test.rb test/file2_test.rb does not work. It consistently executes the first file and nothing else.

The only way I found to execute a bunch of test files together is to run only one them and require the rest (lol). Like this:

ruby -Itest -r./test/file1_test.rb test/file2_test.rb

If you know of another way to do this, please let me know. I couldn't find any alternatives that don't involve rake or other dependencies.

Automated Tests

Adapted our tests to verify the right command.

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock requested review from alexcrocha and st0012 March 20, 2025 21:18
@vinistock vinistock self-assigned this Mar 20, 2025
@vinistock vinistock added server This pull request should be included in the server gem's release notes bugfix This PR will fix an existing bug labels Mar 20, 2025 — with Graphite App
@vinistock vinistock marked this pull request as ready for review March 20, 2025 21:18
@vinistock vinistock requested a review from a team as a code owner March 20, 2025 21:18
@st0012
Copy link
Member

st0012 commented Mar 20, 2025

One hack I found is: ruby -Itest -e "ARGV.each{|f| require f}" test_file1.rb test_file2.rb, which does exactly the same but we don't need to separate 1 file and the rest, so can potentially simplify the implementation?

graphite-app bot pushed a commit that referenced this pull request Mar 20, 2025
…3336)

### Motivation

I noticed that executing entire test directories through the explorer wasn't working. This is due to #3335 and the aspect that we were not automatically discovering the children of test files if they were never expanded.

That meant that, we had no knowledge of their children existing, but when running the tests we are reporting results for those children we know nothing about.

We need to automatically resolve children for test files that haven't been expanded otherwise the items we need to update don't exist.

### Implementation

It's just a matter of invoking our resolve handler if there are no children in the test file item.

### Automated Tests

Added a test.
@vinistock vinistock force-pushed the 03-20-expand_test_directory_command_resolution_with_requires branch from 683a557 to 6dafa89 Compare March 21, 2025 12:49
@vinistock
Copy link
Member Author

Yeah, I liked that. It makes the implementation simpler indeed.

@vinistock vinistock force-pushed the 03-20-expand_test_directory_command_resolution_with_requires branch from 6dafa89 to ff0b173 Compare March 21, 2025 13:07
@vinistock vinistock force-pushed the 03-20-expand_test_directory_command_resolution_with_requires branch from ff0b173 to 9e31b06 Compare March 21, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants