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

3 issue api #44

Merged
merged 7 commits into from
May 26, 2015
Merged

3 issue api #44

merged 7 commits into from
May 26, 2015

Conversation

fanwashere
Copy link
Contributor

No description provided.

@fanwashere
Copy link
Contributor Author

I'm stuck on making the issue specs compatible with the milestone specs.

Issue specs require 2 milestones in the user's own test repo and the organization's test repo. These milestones are constantly being deleted by the Milestone spec. I tried using BeforeAndAfter in Issue spec but it is not behaving as expected.

Ideally, the following should happen:

  1. Issue spec creates the 2x2 required milestones.
  2. Issue spec runs and ends.
  3. Milestone spec removes all milestones.
  4. Milestone spec runs and ends.

@shunjikonishi
Copy link
Contributor

What's the problem?

I checked out this branch, but your code doesn't mixin BeforeAndAfter.
I think you can do avobe things with BeforeAndAfter.

@fanwashere
Copy link
Contributor Author

I added the commit with BeforeAndAfter. It gives either fails half the tests or aborts the suite.

@fanwashere
Copy link
Contributor Author

@shunjikonishi review me.

There is definitely some sort of concurrency issue with the tests, so I created a second test repo (test-repo2) so that it will never conflict with the other specs. One test in OrganizationOpSpec had to be changed to accommodate the second repo.

@shunjikonishi
Copy link
Contributor

Initially, IssueOpSpec failed 5 tests with my local environment,
Next, I could run all test without error by following command

sbt test -Duser.timezone=GMT

So, I guess this is timezone problem. #45
Maybe you could reproduce my error with following command.

sbt test -Duser.timezone=Asia/Tokyo

I think this is another problem.
So, I give you LGTM!

LGTM

@givery givery assigned fanwashere and unassigned shunjikonishi May 26, 2015
@fanwashere
Copy link
Contributor Author

@shunjikonishi review me.

The error is most likely produced by the following line:

var nTime: DateTime = DateTime.now()

Which I use the get the current time so I can compare with the created_at and updated_at time. I added a line to convert your local time into UTC, as I assume Github is on UTC time.

var nTime: DateTime = DateTime.now().toDateTime(DateTimeZone.UTC)

This should solve the error. Please use commit 96088ef to test this.

Also, I noticed the master I used for the base of this branch is outdated and in AbstractJson, the date() function was changed to getDate() recently. The last commit (a8d92d4) uses the new getDate() function. Don't test with this as the branch is still using the date() function.

@shunjikonishi
Copy link
Contributor

@fanwashere fix me

To solve conflict, try following step

$ git checkout master
$ git pull origin master
$ git checkout 3-issueAPI
$ git rebase master
(Maybe conflict happen, so you have to fix them with your editor.)
$ git rebase --continue
$ git push -f origin 3-issueAPI

The points are

  • use 'rebase' to merge master to branch
  • use '-f' option in last git push

This is basic way to merge master to branch.

FYI @sukeshni

Added the last missing method listRepositoryIssue.

Implemented tests for create, get, and edit issue, but not done tests for listing and filtering issues.

listRepositoryIssue method works but untested.
@fanwashere
Copy link
Contributor Author

@shunjikonishi Thanks for the advice, this will be useful in the future as I constantly forget to rebase before I make a branch.

@sukeshni review me.

@givery givery assigned sukeshni and unassigned fanwashere May 26, 2015
@givery givery added Review me! and removed Fix me! labels May 26, 2015
@sukeshni
Copy link
Contributor

LGTM

@givery givery assigned fanwashere and unassigned sukeshni May 26, 2015
fanwashere added a commit that referenced this pull request May 26, 2015
@fanwashere fanwashere merged commit f2c07d3 into master May 26, 2015
@fanwashere fanwashere deleted the 3-issueAPI branch May 26, 2015 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants