Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

tests: only run functional tests when source has changed #1619

Merged
merged 3 commits into from Nov 4, 2015
Merged

tests: only run functional tests when source has changed #1619

merged 3 commits into from Nov 4, 2015

Conversation

steveej
Copy link
Contributor

@steveej steveej commented Oct 16, 2015

Fixes #1513.

@@ -8,6 +8,8 @@ if [ -f /opt/change-go-version.sh ]; then
change-go-version 1.4
fi

SRC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | grep -vE "^Documentation/" | wc -l)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you just exit when there are no code changes?

git diff-tree --no-commit-id --name-only -r HEAD..origin/master | grep -vqE "^Documentation/"
if [ $? != 0 ] ; then
  echo No changes to source detected.
  exit 0
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of adding another check for doc changes later, similar to

DOC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | grep -E "^Documentation/" | wc -l)

and then have another if clause that handles documentation changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

HEAD..origin/master might be Semaphore specific and developers might name their git-remotes differently. Do you think we should check $SEMAPHORE=true and $CI=true? See Semaphore environment variables

fi

# Semaphore does not clean git subtrees between each build.
sudo rm -rf "${BUILD_DIR}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decide to do the rm -rf only in the $CI case, you can add a similar if for the other rm -rf line 75.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We clone to ${BUILD_DIR} regardless, so we have to remove it regardless. The rm from L32 is because we assume that semaphore didn't cleanup after itself. Basically, we should just check if it's there at all and remove it before cloning it.

@alban
Copy link
Member

alban commented Oct 19, 2015

One small thing to fix, LGTM after that.

fi

# In case it wasn't cleaned up
if [ -e "${BUILD_DIR}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check seems redundant?

@jonboulle
Copy link
Contributor

Rebase.

SRC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | \
grep -cEv "^Documentation/")
# We might not need to process the docs
DOC_CHANGES=$(git diff-tree --no-commit-id --name-only -r HEAD..origin/master | \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about catching changes in {ROADMAP,README,CONTRIBUTING,CHANGELOG}.md? These currently will land in SRC_CHANGES, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

@jonboulle
Copy link
Contributor

LGTM after krnowak's comments are addressed. Thanks!

DOC_CHANGES=1 # process docs by default
DOC_CHANGE_PATTERN="\
-e '^Documentation/' \
-e 'README|ROADMAP|CONTRIBUTING|CHANGELOG)|\.md' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semaphore complains about the ):

grep: Unmatched ) or \)

@iaguis
Copy link
Member

iaguis commented Oct 27, 2015

Can you please rebase?

@iaguis
Copy link
Member

iaguis commented Nov 4, 2015

LGTM

iaguis added a commit that referenced this pull request Nov 4, 2015
tests: only run functional tests when source has changed
@iaguis iaguis merged commit e6c571c into rkt:master Nov 4, 2015
@steveej steveej deleted the ci-change-detection branch November 4, 2015 16:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants