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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditionally pass test suites #2028

Closed
wants to merge 13 commits into from

Conversation

@mrcasals
Copy link
Contributor

commented Oct 11, 2017

馃帺 What? Why?

Let's try to conditionally run tests on CircleCI:

  • If we're on master git branch, run all jobs
  • If a file from decidim-core is modified, run all jobs
  • Else, only run tests from the modified folder.

The main tests are always run, because that's where we have the i18n specs and similar. This means that if we only modify files from proposals, then only main and proposals jobs would really run.

To discuss:

  • If a file from the main folder (Gemfile(.lock), Rakefile, Dockerfile, config/, lib/, spec/) is changed, run all jobs? Note that main is always run right now.

馃搶 Related Issues

@mrcasals mrcasals self-assigned this Oct 11, 2017

@ghost ghost added the in-progress label Oct 11, 2017

@mrcasals mrcasals force-pushed the tests/conditionally-pass-tests branch from 0eae0e2 to 9198bac Oct 11, 2017

@codecov

This comment has been minimized.

Copy link

commented Oct 11, 2017

Codecov Report

Merging #2028 into master will decrease coverage by 7.83%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2028      +/-   ##
==========================================
- Coverage   98.59%   90.76%   -7.84%     
==========================================
  Files        1186      469     -717     
  Lines       27385     9603   -17782     
==========================================
- Hits        27000     8716   -18284     
- Misses        385      887     +502
@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2017

The commit called Should only run main,proposals ended up (after some other commits, because I had some failures) running this workflow:

https://circleci.com/workflow-run/5e9a90e2-0aaf-4aa7-9a35-11cae46aa71a

I created a file on decidim-proposals. As you can see, only main and proposals are run.

Then I moved this file to decidim-core in commit This should run all jobs, and this workflow was run:

https://circleci.com/workflow-run/166dc91f-364a-491c-b355-b3f02bd7274b

I'm waiting for the suite to run, but should run all jobs. EDIT: it doesn't, will check later.

(I'm not using commit SHAs because they might change on rebases)

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2017

Awesome, go go @mrcasals!

@beagleknight
Copy link
Contributor

left a comment

Yeaaaah 馃帀

@mrcasals mrcasals force-pushed the tests/conditionally-pass-tests branch from 1e584df to db9002c Oct 11, 2017

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2017

Update: I was dumb and commented out part of the code that needs to run on CircleCI, otherwise the script won't work.

Also, note that my bash knowledge is quite lacking 馃槄

Edit: after fixing the code, it looks like the workflow is working as expected. This one is running all the tests because there are changes on decidim-core:

https://circleci.com/workflow-run/0fea4338-5287-41bd-910b-dc17dced92c5

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2017

@deivid-rodriguez @beagleknight @josepjaume I've updated the description with things to discuss, I like it the way it is right now but I can try to change the behaviour if you guys think it's needed 馃槃

@deivid-rodriguez
Copy link
Contributor

left a comment

If a file from the main folder (Gemfile(.lock), Rakefile, Dockerfile, config/, lib/, spec/) is changed, run all jobs? Note that main is always run right now.

I think what you're currently doing is a good approach for now. We can tweak it further in the future if we feel we need it.

testCommand=$2
branch=`git rev-parse --abbrev-ref HEAD`
echo $branch
modifiedFiles="git diff --name-only $branch master"

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Oct 13, 2017

Contributor

Seems unused, maybe you meant to use it afterwards.


if [ "$branch" = "master" ]; then
echo "YES, IT'S MASTER!!"
eval $testCommand

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Oct 13, 2017

Contributor

Bad indentation


git checkout master
git reset --hard origin/master
git checkout -

This comment has been minimized.

Copy link
@deivid-rodriguez

deivid-rodriguez Oct 13, 2017

Contributor

Would it be possible to you use origin/master instead of master in the subsequent git diff's and thus avoid this?

This comment has been minimized.

Copy link
@mrcasals

mrcasals Oct 13, 2017

Author Contributor

Let me try! I had to use all this because it seems that, when pulling, CircleCI only pulls the branch it's building, so when I compare to master I had no changes because git was creating master from the current branch. That's why I wrote this piece of code. Let me see if your solution works :)

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

@deivid-rodriguez tried your solution and works nice, thanks! I've found an issue which might have been there in my original code. This PR is not rebased to current master branch, and when checking the files changes from this branch against origin/master I see this:

Screenshot from a CircleCI build accessing there through SSH:

As you can see, the list of changed files include the files that are on origin/master. Do you have any idea on how to get the list of commits from a given branch, and see the files modified by those commits? I guess this is what we need to do 馃槙

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

Do you have any idea on how to get the list of commits from a given branch, and see the files modified by those commits? I guess this is what we need to do 馃槙

No... I guess you need to find the base commit for the PR somehow... 馃 But I don't think this is a big issue for now. I mean, it's still an improvement over what we have, and you can always rebase your PR if you want the list to be 100% correct.

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

$ git log --oneline --no-merges --left-only --name-only --pretty='format:' $branch...origin/master

This shows, for each commit, a list of the names of the files that have been modified:

Still, I left the command as:

$  git diff --name-only origin/master...$branch

Which returns this:

I don't know what happens if the same file is modified both in master and in the branch, but I guess it's OK by now

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

@mrcasals Nice!

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

We run into Shopify/bootsnap#87 apparently. Not sure how to proceed.

@josepjaume
Copy link
Contributor

left a comment

Super nice!!

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

@deivid-rodriguez not the first time I see it, it usually goes away when rebuilding the job (I know it's not ideal tho)

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

Ok! I was wondering if this had just appeared now since I saw in the logs version 1.1.5 being installed.

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

Note: we'll be merging this on Monday so we can control master better 馃槃

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

@deivid-rodriguez codecov is being hung up, I think this is because codecov data is calculated when running the test suite:

https://github.com/codecov/codecov-ruby

https://github.com/decidim/decidim/blob/master/decidim-dev/lib/decidim/dev/test/base_spec_helper.rb#L10-L17

I don't know if that's really the cause, but if it is then I don't know how to fix it :(((((((

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

@mrcasals Arrrgh, yes. Is it somehow expecting code coverage results from all engines or something? Did it work in the other experiments when all engine tests were supposed to be run?

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2017

@deivid-rodriguez commit d3b6ecf has all jobs running, and codecov checks passed (note there are 18 checks).

Next commit, a046a94, removed the test file, which should only run main. Here there are only 16 checks, codecov missing, even though they are checked as required: 馃槙

My guess is that we should separate each engine in a different project in codecov, and send the reports for each project if needed. Does this make sense? Problem is, I don't know if codecov is ready to handle such an architecture, so we might be blocked here 馃槶 We can try to ask though.

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2017

@mrcasals What you suggest makes sense to me as an alternative.

@beagleknight
Copy link
Contributor

left a comment

Awesome! :)

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2017

This might be related to our problems: https://github.com/codecov/support/issues/355 I'll investigate a little bit more and open an issue in the repo if everything fails

@mrcasals mrcasals dismissed stale reviews from beagleknight and josepjaume via 6a676af Oct 16, 2017

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2017

We could also leave only relative coverage enabled, since the problems happen only with global (project) coverage, right?

I'm unsure what do to here. The best for us would be that CircleCI would get build auto-cancellation working (https://discuss.circleci.com/t/auto-cancel-redundant-builds-not-working-for-workflow/13852), since it's when crowdin starts to compulsively commit when the builds start getting queued up after the crowdin branch.

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

@deivid-rodriguez yeah but the problem persists even if the auto-cancel works (note that during office hours I usually keep cancelling redundant builds on CircleCI).

I contacted CodeClimate, as they released a beta TestCoverage feature, to see if their implementation would help us. I copied my email in a gist, waiting for their answer:

https://gist.github.com/mrcasals/4f34f2a64f4ad389e5b2e3b8d0c8f6bc

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2017

I see, didn't know you were doing the dirty work of cancelling redundant builds...

What about the idea a living without project coverage for now? According to my experience, the most useful check for PR's is actually relative coverage.

@mrcasals

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

CodeClimate reported back: codeclimate/test-reporter#257 (comment)

TLDR: we could generate the reports per job, and cache them with the name of the branch, eg coverage-proposals-my-feature-branch. Once all tests are run, we would need to add a new job that restored all reports from the current branch, with a fallback to master, calculate the reports and upload them.

If we wanted to use CodeClimate test coverage right now we'd need to do this anyway (without the part of fallbacking to master), so I'd like to try this.

@mrcasals mrcasals force-pushed the tests/conditionally-pass-tests branch from ac54c2a to a4b7aa0 Nov 13, 2017

@mrcasals mrcasals force-pushed the tests/conditionally-pass-tests branch from 96bd68e to e0fe587 Nov 14, 2017

mrcasals added some commits Oct 11, 2017

f
Try to keep the code simpler
Should only run main,proposals
Improve command to check for files
Runs main,proposals
Move test file to core
Should run all jobs

@mrcasals mrcasals force-pushed the tests/conditionally-pass-tests branch from e0fe587 to d3dfe33 Nov 14, 2017

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

I wonder if we should do this at this point. The build is back to reasonably fast, and having everything run on every PR makes me sleep better at night. We still have some inter-gem dependency after all.

@josepjaume

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

After talking about it with @mrcasals, we agreed that this isn't such a good idea. It's prone to errors and it's digging down the rabbit hole - if we really think the libraries are that independent from each other, then let's make that explicit by splitting it into several repositories.

I already have some ideas with how to version libraries together so they have some space to grow independently feature-wise while keeping a contract with the core.

@josepjaume josepjaume closed this Nov 22, 2017

@ghost ghost removed the in-progress label Nov 22, 2017

@deivid-rodriguez deivid-rodriguez referenced this pull request Nov 22, 2017

@oriolgual oriolgual deleted the tests/conditionally-pass-tests branch Dec 5, 2017

@mrcasals mrcasals referenced this pull request Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.