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

Compare time.Time objects for equality correctly #979

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

senyast4745
Copy link
Contributor

Adding test

Summary

Fix assert.ObjectsAreEqual bug with not equals time.Time before and after serializing\deserializing

Changes

Added a check in ObectsAreEqual in case the arguments are a time object

Motivation

Fix bug to use test in docker

Related issues

Closes #950
Fix #951

@senyast4745
Copy link
Contributor Author

@leoleoasd , did you talk about it?

@leoleoasd
Copy link

leoleoasd commented Jul 20, 2020

The problem is that, this PR repeats two commits from master branch.
image
the "red commits" are commits of master branch, and, as you can see, your branch repeats these two commits.

Also, the "rebase" command changes the "base commit" of your branch. as you can see, your branch starts from "38a7ed4", which is not the latest commit of master, so there will be conflicts.

In my opinion, what you should do is :

# on bugfix/time-equals-2 branch
git reset f41f6c2

# remote origin should be your fork
# we need to add a new origin
# skip if you already done
git remote add upstream https://github.com/stretchr/testify.git
git fetch upstream # fetch the upstream changes
git rebase upstream/master # rebase your current branch (bugfix/time-equals-2)
git push -f

This will undo your "repeated" commits, rebase your branch from the latest master, and force-push. After doing these, your git log should be like this:
image
No bifurcations anymore and your commits are "based on" the lastest upstream master.

@senyast4745
Copy link
Contributor Author

@leoleoasd check please last update

@leoleoasd
Copy link

LGTM, but I don't have write access to this repo, we need to wait for a reviewer with write access 😂

@senyast4745
Copy link
Contributor Author

I know)

@leoleoasd
Copy link

@boyan-soubachov could you please review this?

@boyan-soubachov boyan-soubachov changed the title Bugfix/time equals 2 Compare time.Time objects for equality correctly Jul 27, 2020
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you :)

@senyast4745
Copy link
Contributor Author

senyast4745 commented Jul 27, 2020

@leoleoasd merged)) Thank you

leoleoasd added a commit to leoleoasd/testify that referenced this pull request Jul 27, 2020
leoleoasd added a commit to leoleoasd/testify that referenced this pull request Jul 27, 2020
@titanous
Copy link

It looks like this was reverted in ed4976c?

@leoleoasd
Copy link

It looks like this was reverted in ed4976c?

Yes, check #985 for more details.
If you can't wait for v2, you can use my fork.

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.

assert.ObjectsAreEqual work not correct with serialized\deserialized time.Time
4 participants