opendev/gerrit
Code Issues Proposed changes

Fix flaky test that relied on non-stable order of approvals

Browse Source 
The order in which the approvals are returned can differ from machine
to machine. Adapt the test assertions in
testPushForMasterWithApprovalsForgeCommitterButNoForgeVote so that the
order of the returned approvals doesn't matter as the order is of no
importance for this test.

In addition remove the definition of the custom label since it is not
required for testing the correct behaviour. Having it was just leading
to a better error in case that the fix was not there. The test still
fails if the fix is missing, but with a different error. Removing the
custom label may also make the test more stable since with multiple
labels it may not be deterministic on which label the dummy vote for
the committer is added.

Change-Id: I5a4b008a30cfc89c91aa71e09e781246246a0394
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2015-10-15 09:30:51 +02:00
parent ff1a3e49ae
commit 3dcf53f214
1 changed files with 10 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@@ -19,8 +19,6 @@ import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.GitUtil.cloneProject;
import static com.google.gerrit.acceptance.GitUtil.createCommit;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
@@ -31,7 +29,6 @@ import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.GitUtil.Commit;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -40,8 +37,6 @@ import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gwtorm.server.OrmException;
@@ -259,36 +254,15 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
* There was a bug that allowed a user with Forge Committer Identity access
* right to upload a commit and put *votes on behalf of another user* on it.
* This test checks that this is not possible, but that the votes that are
* specified on push are applied only in the name of the uploader.
* specified on push are applied only on behalf of the uploader.
*
* This particular bug only occurred when there was more than one label
* defined. Hence the test defines a custom label.
*
* When on upload the committer is forged he is automatically added as
* reviewer to the change. This results in a dummy 0 vote. If there was only
* one label this dummy 0 vote collided with any vote for the same label that
* was specified in the push specification and hence the upload failed, which
* means in this case it was not possible to forge a vote.
* defined. However to test that the votes that are specified on push are
* applied on behalf of the uploader a single label is sufficient.
*/
@Test
public void testPushForMasterWithApprovalsForgeCommitterButNoForgeVote()
throws GitAPIException, IOException, RestApiException {
// add custom label because the bug only allowed to forge a vote when there
// were at least two labels
LabelType Q = category("CustomLabel",
value(1, "Positive"),
value(0, "No score"),
value(-1, "Negative"));
ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
cfg.getLabelSections().put(Q.getName(), Q);
MetaDataUpdate md = metaDataUpdateFactory.create(allProjects);
try {
cfg.commit(md);
} finally {
md.close();
}
projectCache.evict(allProjects);
throws GitAPIException, RestApiException {
// Create a commit with "User" as author and committer
Commit c = createCommit(git, user.getIdent(), PushOneCommit.SUBJECT);
@@ -305,10 +279,12 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
ChangeInfo ci = get(c.getChangeId());
LabelInfo cr = ci.labels.get("Code-Review");
assertThat(cr.all).hasSize(2);
assertThat(cr.all.get(0).name).isEqualTo("Administrator");
assertThat(cr.all.get(0).value.intValue()).is(1);
assertThat(cr.all.get(1).name).isEqualTo("User");
assertThat(cr.all.get(1).value.intValue()).is(0);
int indexAdmin = admin.fullName.equals(cr.all.get(0).name) ? 0 : 1;
int indexUser = indexAdmin == 0 ? 1 : 0;
assertThat(cr.all.get(indexAdmin).name).isEqualTo(admin.fullName);
assertThat(cr.all.get(indexAdmin).value.intValue()).is(1);
assertThat(cr.all.get(indexUser).name).isEqualTo(user.fullName);
assertThat(cr.all.get(indexUser).value.intValue()).is(0);
assertThat(Iterables.getLast(ci.messages).message).isEqualTo(
"Uploaded patch set 1: Code-Review+1.");
}