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

CI::Queue::Static and CI::Queue::File initialization is broken #42

Open
rafaelfranca opened this issue Nov 22, 2017 · 3 comments
Open

Comments

@rafaelfranca
Copy link
Member

After #34, you need to initialize the File and Static queue passing the file name or the test respectively and you also need to populate the queue with the list of tests. This seems contra-intuitive.

Maybe the initialize should only take the config and the populate the list of tests and add a .populate class method (or any other name) that does what the initialize used to do.

Also those queues require a configuration object that is not needed in most of its use cases. Making it optional, or adding a high level method as .populate that creates the configuration object would be great.

cc @casperisfine @wvanbergen @tjoyal


Nitpick:

The @index instance variable is only set after you call #populate so if you don't you get a "non-method error fetch for nil" in the to_a method and a ruby warning because the instance variable is not initialized. Maybe we should try to write warning free code so we could avoid this kind of error?

@casperisfine
Copy link
Contributor

I understand it might seem awkward, but it's on purpose.

All Queue implementations (Static, File, etc) should follow the same interface, so they're a bit more complicated to use than really necessary because they have to mimick the redis queue API.

Making it optional, or adding a high level method as .populate [...]

We could certainly offer some higher level methods for convenience.

The @Index instance variable is only set after you call #populate

Same here, I could check for that and raise a cleaner error.

@rafaelfranca
Copy link
Member Author

I totally see this API change as being a good thing. But do we need to pass the file or the list of test to the initialize of Static and File? It doesn't seems to be used. So maybe we can pass only to populate.

@casperisfine
Copy link
Contributor

It doesn't seems to be used.

It actually is. TBH I'm not 100% happy with all this because I try to make those 3 class share the same interface but they work quite differently internally.

For Static and File, the populate doesn't define which tests will be ran, it simply gives the actual test objects.

e.g.

all_tests = [
  OpenStruct.new(name: 'a'),
  OpenStruct.new(name: 'b'),
  OpenStruct.new(name: 'c'),
  OpenStruct.new(name: 'd'),
]

queue = Static.new(%w(a b))
queue.populate(all_tests, &:name)
queue.poll do |test|
  p test.name
end

will print

"a"
"b"

But ultimately users shouldn't have to mess with those internals, you had to because of the bisect script in core, but I'm working on bringing it inside the gem: #43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants