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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flags to common schema payload #857

Merged
merged 2 commits into from
Oct 31, 2018
Merged

Conversation

guperrot
Copy link
Member

@guperrot guperrot commented Oct 31, 2018

Please have a look at our guidelines for contributions and consider the following before you submit the PR:

  • Has CHANGELOG.md been updated?
  • Are tests passing locally?
  • Are the files formatted correctly?
  • Did you add unit tests?
  • Did you test your change with either the sample apps that are included in the repository or with a blank app that uses your change?

Description

A few sentences describing the overall goals of the pull request.

Related PRs or issues

List related PRs and other issues.

Misc

Add what's missing, notes on what you tested, additional thoughts or questions.


/* Verify reset of epoch/seq. */
/* Verify flags and reset of epoch/seq. */
assertEquals(new Long(PERSISTENCE_NORMAL), log4.getFlags());
assertEquals((Long) 1L, log4.getExt().getSdk().getSeq());
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use only one way for casting numbers in asserts like this. For example

Suggested change
assertEquals((Long) 1L, log4.getExt().getSdk().getSeq());
assertEquals(Long.valueOf(1), log4.getExt().getSdk().getSeq());

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok.

@@ -92,18 +95,18 @@ public void enqueueConvertedLogs() {
when(originalLog.getTransmissionTargetTokens()).thenReturn(new HashSet<>(Collections.singletonList("t1")));

/* Mock a log. */
CommonSchemaLog log1 = mock(CommonSchemaLog.class);
Copy link
Contributor

@thyeggman thyeggman Oct 31, 2018

Choose a reason for hiding this comment

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

I know this is not necessarily in scope for this PR, but the enqueueConvertedLogs test is doing a lot. I'd like to see it split up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how to split as we need to test reset the sequence of epoch and seq in the same test.

@guperrot guperrot changed the base branch from feature/priority-channel to feature/priority October 31, 2018 19:09
@guperrot guperrot merged commit 0b410a7 into feature/priority Oct 31, 2018
@guperrot guperrot deleted the feature/cs3-flags branch October 31, 2018 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants