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

Created issue_589 spec file and yaml #590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

babaliaris
Copy link

#589
This does not contain the fix yet, just a unit test to replicate the problem. In other words, the test should fail for now until the bug has been fixed.

The problem should be in this line but I wasn't able to replicate the problem in the project as I did in the distributed npm module (see #589 original post Examples and context).

I just created a simple test before trying to fix the bug and the result is this:
(Please note I'm in a Windows environment and I use npm run test:windows)

 1) DateFormats
       should get the correct date:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '1996-01-03T22:00:00.000Z'
- '1996-01-04'
            ^
      + expected - actual

      -1996-01-03T22:00:00.000Z
      +1996-01-04

      at A:\NodeProjects\express-openapi-validator\test\issue_589.spec.ts:32:16
      at processTicksAndRejections (internal/process/task_queues.js:93:5)

The +actual in the assertion should be 1996-01-03 (this is what I get while using the published npm version) and not 1996-01-03T22:00:00.000Z
Even if I completely change the behavior of this line the test result would still be the same which is weird.

Maybe it's not this line that causes the problem?

Suggested Fix:

//d MUST be a Date Object.
const d = d.getTimezoneOffset();

//This will make sure to advance the day, month or year
//of a UTC time in such a way that the time part will be zero
//Example: "1996-01-03T22:00:00.000Z" ====> "1996-01-04T00:00:00.000Z"
d = new Date(yourDate.getTime() - (offset*60*1000));

//So now you can safely split using 'T' and keep the first part.
const correct_YYYY_MM_DD = d.toISOString().split('T')[0];

@cdimascio
Copy link
Owner

cdimascio commented May 3, 2021

@babaliaris this test look incorrect.
you are providing a ISO utc date, toISOString() is called on the date and the date portion is retained. The current behavior looks correct.

@babaliaris
Copy link
Author

@babaliaris this test look incorrect.
you are providing a ISO utc date, toISOString() is called on the date and the date portion is retained. The current behavior looks correct.

The test request in this line uses a similar way as the code snippet in issue 589.

Also in the YAML file this line I'm making sure it is a date, not DateTime. Only DateTime should return a full iso 8601 string. Currently, my test gets a full iso 8601 string while in the YAML file I described the date field as a date format so it's weird that I'm taking a full iso 8601 string in the response.

So the result should be 1996-01-03 , the same result I'm getting in issue 589.

I truly can't understand why I'm getting this result. If you already know how to fix my issue 589, then you are free to do it. Unfortunately, I can't figure it out. I'm really sorry.

mdwheele pushed a commit to mdwheele/express-openapi-validator that referenced this pull request Jan 6, 2022
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.

2 participants