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

Factor regex error messages with spec API tests #8251

Merged
merged 1 commit into from Nov 12, 2014

Conversation

5 participants
@cirosantilli
Contributor

cirosantilli commented Nov 5, 2014

@TeatroIO

This comment has been minimized.

TeatroIO commented Nov 5, 2014

I've prepared a stage. Click to open.

@cirosantilli cirosantilli changed the title from Factor regex error messages with spec API tests to [WIP] Factor regex error messages with spec API tests Nov 5, 2014

@cirosantilli cirosantilli force-pushed the cirosantilli:factor-regex-message-spec branch from 832ed31 to 98db90c Nov 5, 2014

@cirosantilli cirosantilli changed the title from [WIP] Factor regex error messages with spec API tests to Factor regex error messages with spec API tests Nov 5, 2014

@Razer6 Razer6 added this to the 7.5 milestone Nov 7, 2014

@Razer6

This comment has been minimized.

Member

Razer6 commented Nov 7, 2014

/cc @maxlazio

@maxlazio

This comment has been minimized.

Member

maxlazio commented Nov 7, 2014

@cirosantilli Can you explain your reasoning behind this? In this case, I don't think that DRY-ing up is good. IMO, DRY-ing up here makes the tests difficult to understand.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Nov 7, 2014

@maxlazio I see what you mean, and I'm not an expert on testing best practices, but I'll give my 2c.

We already try to keep our tests DRY by using things like RSpec subjects, let, shared_group, factories, variables, methods, etc. extensively and I think everyone agrees we should use them.

Those things do make tests DRYer and harder to understand, but we still use them. Isn't it analogous to what is done here? Or to any DRYing up done in the code itself?

@maxlazio

This comment has been minimized.

Member

maxlazio commented Nov 12, 2014

@cirosantilli Yes, but all of those are test code written specifically for tests. They are not used in production code. We use tests to test the production code. If we use production code to test the production code we go into a bit of a loop and possible problem, don't you agree?

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Nov 12, 2014

@maxlazio if we have a production function f() and use the function on a test, the loop is broken by adding a unit test to function f().

Here f() = Constant: so we could add a single test like:

Gitlab::Regex.send(:default_regex_message).should eq('abc')

and use Gitlab::Regex.send(:default_regex_message) on the entire test suite. The only reason we don't is because that test would be trivial, and because checking that the test:

Gitlab::Regex.send(:default_regex_message).should eq('abc')

is correct, comes down exactly to looking up what it is on the source code, which is the same that readers the test on the current PR would have to do.

dzaporozhets added a commit that referenced this pull request Nov 12, 2014

Merge pull request #8251 from cirosantilli/factor-regex-message-spec
Factor regex error messages with spec API tests

@dzaporozhets dzaporozhets merged commit 1dc99b4 into gitlabhq:master Nov 12, 2014

1 check passed

default The build passed on Semaphore.
Details

@cirosantilli cirosantilli deleted the cirosantilli:factor-regex-message-spec branch Nov 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment