requests/api/project_spec.rb speed up #8445
Conversation
|
I've prepared a stage. Click to open. |
| it { json_response['permissions']["project_access"]["access_level"].should == Gitlab::Access::MASTER } | ||
| it { json_response['permissions']["group_access"].should be_nil } | ||
| expect(response.status).to eq(200) | ||
| expect(json_response['permissions']["project_access"]["access_level"]). |
There was a problem hiding this comment.
Line is too long. [81/80]
Prefer single-quoted strings when you don't need string interpolation or special symbols.
There was a problem hiding this comment.
Going to ignore the Line is too long notice, since i have to perform some super weird tricks to get that one char win.
jvanbaarsen
force-pushed
the
project-spec-speed-up
branch
2 times, most recently
from
3d1c9be
to
c8e732a
Compare
jvanbaarsen
changed the title
|
@dblessing Can you review? |
| end | ||
|
|
||
| context 'group project' do | ||
| before do | ||
| it 'should set the owner and return 200' do |
There was a problem hiding this comment.
I think the style that was present before is preferred. If you want to only evaluate the before once to speed things up and check a bunch of things, use before(:all) instead of the before(:each) that's implied. As much as possible I think we should only assert one thing per it block.
There was a problem hiding this comment.
I think having one assertion per test is good when you're writing your code. after you're done writing, the tests are just there to validate behaviour. I think its best to move assertion that can be handled in one test to a single test, this speeds up significantly on the long term. Since the current test run takes over 19 minutes, every speed up is welcome.
There was a problem hiding this comment.
I don't think you can or should change the style of a test based on pre/post code writing. The best practice stands, IMHO. I think if we use 'before(:all)' here we will achieve good test style and get the speed up because the before block will only be evaluated once.
There was a problem hiding this comment.
This is purely a style preference. I'm not nearly as concerned about this one. :)
There was a problem hiding this comment.
@randx Curious about your opinion on this :)?
There was a problem hiding this comment.
Ping me when you push up changes.
There was a problem hiding this comment.
@dblessing Just tried the before(:all) approach, but this will be deprecated as of rspec 3, since @NARKOZ is working on upgrading rspec, I think it would be best to keep the change as I have proposed initially. What do you think?
There was a problem hiding this comment.
Sounds good. I will take one last look at merge this.
There was a problem hiding this comment.
Seems like before(:all) is not deprecated AFAICT. I will still move forward with this PR, though.
There was a problem hiding this comment.
Sorry, i should have been more precise :) It is deprecated in combination with using a let.
|
@dblessing Can this be marked as ready for merge? Or you want opinion of @randx ? |
|
If you dont mind I leave decision to @dblessing. Feel free to merge it if you think this is ready. |
|
@randx Ok thanks! Will fix the last items later tonight. |
|
@dblessing After this gets rebased, would you be confortible with it being merged? |
|
yes, looks good. |
jvanbaarsen
force-pushed
the
project-spec-speed-up
branch
2 times, most recently
from
446dd4c
to
22d1265
Compare
| (1..user2.projects_limit-1).each { |p| post api('/projects', user2), name: "foo#{p}" } | ||
| post api('/projects', user2), name: 'foo' | ||
| it 'should create new project without path and return 201' do | ||
| expect { post api("/projects", user), name: 'foo' }.to change {Project.count}.by(1) |
There was a problem hiding this comment.
Line is too long. [89/80]
Space missing inside {.
Space missing inside }.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
jvanbaarsen
force-pushed
the
project-spec-speed-up
branch
from
22d1265
to
68abfcb
Compare
|
@dblessing Somehow two commits from master got in this PR, and im not really sure how to get them out (already tried rebasing). You have any ideas? If not I'll cherry-pick my commit into a clean branch and recreate a PR. |
|
I see that. Odd. Rebasing should have taken care of it. I don't know another way unfortunately. |
|
@dblessing Hm ok, I'll cherry pick into another branch then :) |
Signed-off-by: Jeroen van Baarsen <jeroenvanbaarsen@gmail.com>
jvanbaarsen
force-pushed
the
project-spec-speed-up
branch
from
68abfcb
to
0ef4957
Compare
|
@dblessing I think that GitHub just had a hickup or something, I rebased again and now all is well :) When this is green I'll merge. |
|
👍 for merging this. I also noticed this test case was unnecessarily slow. |
|
Thanks for the reminder. Totally forgot about this one On Wed, Feb 11, 2015 at 8:28 PM, Robert Speicher notifications@github.com
|
requests/api/project_spec.rb speed up
What does this MR do?
It improves the speed of the
request/api/project_spec.rbtests by ~3 minutes. It also removes some test duplication, and moves some assertions into the same tests so another API call is not needed.Before:
~3.51 minAfter:
~ 50 sec.