-
-
Notifications
You must be signed in to change notification settings - Fork 79
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
Normalize http examples #1215
Normalize http examples #1215
Conversation
Let plone-5.2.x.cfg first extend the default Plone 5.2.5 versions, and then base.cfg. Let base.cfg contain version overrides, including `plone.restapi =` to unpin ourselves. Also remove version pin from buildout.cfg which are already the same or newer in Plone 5.2.5.
We already ignored this when checking if there are any differences at all. But when there are differences, we still showed *all* differences, including white space, which makes it hard to see the real problems.
Locally I also get a different 'created' date: '1995-07-31T19:30:00' instead of '1995-07-31T17:30:00', as I am in GMT+2, even though the tests set the timezone to UTC. Remaining problem: the token in this same snippet is random. Also, in a few of the other files, instead of 'plone.dexterity.schema.generated.plone_0_Document' I get: 'plone.dexterity.schema.generated.plone_5_1630600102_2_680259_0_Document' I saw these before locally, but assumed it was something in my setup, because the GH Actions kept passing. That seems to have been because on old release of plone.restapi was being tested.
We need to pin an older dexterity because the new dynamic schemas conflict with the expected output in our http-examples. See #1096
…hanges in CI." This reverts commit 6b265ff.
This reverts commit def2340.
…ument_crud. plone.dexterity 2.10+ has dynamic schemas, so we get for example plone.dexterity.schema.generated.plone_5_1630611587_2_523689_0_Document as behavior name. Instead, we save what we got in older dexterity: plone.dexterity.schema.generated.plone_0_Document
The first part of save_request_and_response_for_docs was an exact copy save_request_for_docs (before my change in the previous commit).
…mparison. This avoids many uninteresting differences.
This avoids differences when running from the Plone coredev buildout, which uses a random port. A few differences remain, at least when running the test in Plone 6. On Plone 5.2, only workingcopy_baseline_get.resp has a difference, and this depends on your timezone.
This is a very big diff, but 'git diff -w' is empty. Note: when you view the diff on github, you can add '?w=1' after the url to ignore whitespace differences.
This comment has been minimized.
This comment has been minimized.
@jenkins-plone-org please run jobs |
"layout": "document_view", | ||
"lock": { | ||
"created": "1995-07-31T17:30:00", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny thing is that locally for me, only this line got different. It stayed:
"created": "1995-07-31T14:30:00",
It seems that only in this part the TZ environment variable had no effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
humm this is explained here:
@mauritsvanrees wouldn't it be the case to normalize this too, so as not to show differences locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, but I wonder why the timezone has no effect here.
@avoinea This is the created date of a lock. Can you run the tests locally and then check if this results in a local change in git?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really weird that all dates respect the TZ
environment variable and only this date doesn't.
I saw that who sets this date is the function:
plone.restapi/src/plone/restapi/services/locking/__init__.py
Lines 18 to 19 in df3dba9
def creation_date(timestamp): | |
return datetime.fromtimestamp(timestamp).isoformat() |
Maybe this can help:
@avoinea FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def normalize_test_port(value): | ||
# When you run these tests in the Plone core development buildout, | ||
# the port number is random. Normalize this to the default port. | ||
return re.sub(r"localhost:\d{5}", "localhost:55001", value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we can remove this line:
Line 74 in c284b56
os.environ['ZSERVER_PORT'] = '55001' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove that yes, if we want to.
@sneridagh You made commit 799e11e two years ago to always use port 55001. Was that to make the http-examples always have the same port? In that case we can now remove this setting, if we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the documentation resp
files always get the same value and do not change over builds, it's ok.
document_schema_re = re.compile( | ||
r"^plone.dexterity.schema.generated.plone_.*_Document$" | ||
) | ||
stable_behavior = "plone.dexterity.schema.generated.plone_0_Document" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think stable_behavior should look like what it is in plone.dexterity 2.10+. That is:
plone.dexterity.schema.generated.plone_5_1630600102_2_680259_0_Document
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically that would be closer to what it looks like. But it would be uglier, so I prefer this clean one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even being “uglier”, I prefer it closer to reality. Each part of this code has a meaning. When someone unsuspecting consults the documentation, they can get confused.
Note: this builds on my approved PR #1214 so that one should be merged first.
This normalises the requests and responses that are written to the
http-examples
directory:plone.dexterity
from Plone 5.2.5.?w=1
to the url to ignore whitespace changes, making this easier to review.