Failed validation: #/definitions/QuoteResponse

pudgereyem commented 3 years ago

Issue

Yahoo's quote endpoint returns different data depending on what time a day it is, e.g quote.marketState is a string that described the state of the market. In the current version it's allowed to be either "CLOSED" or "PREPRE". However, at the time of this writing - when the market is closed - quote.marketState was set to "POST" and then later it was set to "POSTPOST".

How to replicate

Run the integration tests for the quote module when the markets (e.g NASDAQ) are closed. Please also make sure that you disable caching by turning of devel mode for the "passing validation" tests (see code here).

Please note that the response from Yahoo is different depending on the time of day, so make sure that you test this when the markets are closed, e.g 20:00 New York Time.

npm run test --t quote.spec.ts

 FAIL  src/modules/quote.spec.ts
  quote
    ✓ allows blank options (7 ms)
    ✓ returns an array for an array (4 ms)
    ✓ returns single for a string (1 ms)
    ✓ throws on unexpected result (5 ms)
    passes validation
      ✕ AAPL (419 ms)
      ✓ OCDO.L (70 ms)
      ✕ BABA (79 ms)
      ✕ QQQ (73 ms)
      ✕ 0P000071W8.TO (77 ms)

Click to see the error for AAPL

``` The following result did not validate with schema: #/definitions/QuoteResponse at Object.validate [as default] (src/lib/validateAndCoerceTypes.ts:140:15) console.dir [ { keyword: 'enum', dataPath: '/0/marketState', schemaPath: '#/definitions/QuoteEquity/properties/marketState/enum', params: { allowedValues: [Array] }, message: 'should be equal to one of the allowed values' }, { keyword: 'enum', dataPath: '/0/marketState', schemaPath: '#/definitions/QuoteEtf/properties/marketState/enum', params: { allowedValues: [Array] }, message: 'should be equal to one of the allowed values' }, { keyword: 'const', dataPath: '/0/quoteType', schemaPath: '#/definitions/QuoteEtf/properties/quoteType/const', params: { allowedValue: 'ETF' }, message: 'should be equal to constant' }, { keyword: 'enum', dataPath: '/0/marketState', schemaPath: '#/definitions/QuoteMutualfund/properties/marketState/enum', params: { allowedValues: [Array] }, message: 'should be equal to one of the allowed values' }, { keyword: 'const', dataPath: '/0/quoteType', schemaPath: '#/definitions/QuoteMutualfund/properties/quoteType/const', params: { allowedValue: 'MUTUALFUND' }, message: 'should be equal to constant' }, { keyword: 'anyOf', dataPath: '/0', schemaPath: '#/anyOf', params: {}, message: 'should match some schema in anyOf' } ] { depth: 4, colors: true } at src/lib/validateAndCoerceTypes.ts:112:26 console.dir [ { language: 'en-US', region: 'US', quoteType: 'EQUITY', quoteSourceName: 'Nasdaq Real Time Price', triggerable: true, currency: 'USD', exchange: 'NMS', shortName: 'Apple Inc.', longName: 'Apple Inc.', postMarketPrice: 135.98, postMarketChange: -0.02999878, regularMarketChange: -0.90000916, regularMarketChangePercent: -0.65737283, regularMarketTime: 1612904402, regularMarketPrice: 136.01, regularMarketDayHigh: 137.877, regularMarketDayRange: { low: 135.85, high: 137.877 }, regularMarketDayLow: 135.85, regularMarketVolume: 73445499, regularMarketPreviousClose: 136.91, bid: 135.84, ask: 135.85, bidSize: 13, askSize: 29, fullExchangeName: 'NasdaqGS', financialCurrency: 'USD', regularMarketOpen: 136.62, averageDailyVolume3Month: 106252653, averageDailyVolume10Day: 85075233, fiftyTwoWeekLowChange: 82.8575, fiftyTwoWeekLowChangePercent: 1.5588636, fiftyTwoWeekRange: { low: 53.1525, high: 145.09 }, fiftyTwoWeekHighChange: -9.080002, fiftyTwoWeekHighChangePercent: -0.06258186, fiftyTwoWeekLow: 53.1525, fiftyTwoWeekHigh: 145.09, dividendDate: 2021-02-11T00:00:00.000Z, earningsTimestamp: 2021-01-27T16:30:00.000Z, earningsTimestampStart: 2021-04-28T10:59:00.000Z, earningsTimestampEnd: 2021-05-03T12:00:00.000Z, trailingAnnualDividendRate: 0.807, trailingPE: 36.88907, trailingAnnualDividendYield: 0.005894383, epsTrailingTwelveMonths: 3.687, epsForward: 4.66, epsCurrentYear: 4.45, priceEpsCurrentYear: 30.564045, sharesOutstanding: 16788100096, bookValue: 3.936, fiftyDayAverage: 133.41939, fiftyDayAverageChange: 2.5906067, fiftyDayAverageChangePercent: 0.019417018, twoHundredDayAverage: 120.43009, twoHundredDayAverageChange: 15.579903, twoHundredDayAverageChangePercent: 0.12936886, marketCap: 2283349475328, forwardPE: 29.186695, priceToBook: 34.555386, sourceInterval: 15, exchangeDataDelayedBy: 0, tradeable: false, firstTradeDateMilliseconds: 1980-12-12T14:30:00.000Z, priceHint: 2, postMarketChangePercent: -0.022056306, postMarketTime: 2021-02-10T00:59:45.000Z, messageBoardId: 'finmb_24937', exchangeTimezoneName: 'America/New_York', exchangeTimezoneShortName: 'EST', gmtOffSetMilliseconds: -18000000, market: 'us_market', esgPopulated: false, marketState: 'POSTPOST', displayName: 'Apple', symbol: 'AAPL' } ] { depth: 4, colors: true } at src/lib/validateAndCoerceTypes.ts:112:26 ```

Proposed Solution

Since the response from Yahoo's /quote-endpoint can return a response where marketState is set to "POST" or "POSTPOST" we should add these ass possible values for the marketState property for the QuoteBase interface.

export interface QuoteBase {
  ...
  // marketState: "CLOSED" | "PREPRE"; // <-- current 
  marketState: "CLOSED" | "PREPRE" | "POST" | "POSTPOST"; // <-- suggested
  ...
}

Comments / Discussion

Caching the responses can hide issues like this where the response from Yahoo differs depending on the time of day

Since Yahoo's response is different depending on the time of the day, it's important that while in development we turn of devel mode every now and then. I was thinking that we could add parameter support to the test script so that you could run the tests with devel mode turned of, maybe something like this:

yarn test --live

pudgereyem commented 3 years ago

@gadicc I'll create a PR according to the proposed solution above, but I'd love to hear your thoughts on the yarn test --live option. I'll also try to run the tests with caching turned at different times to see if there are more issues like this.

pudgereyem commented 3 years ago

@gadicc please see PR that fixes the issue with marketState here; https://github.com/gadicc/node-yahoo-finance2/pull/33.

Let's create a separate issue for how we best deal with running the test suite with devel mode turned off. Sadly you can't pass custom arguments to jest, so maybe the best option would be to add environment variables through .env which we could take into account in yahooFinanceFetch.js possibly?

gadicc commented 3 years ago

Thanks, @pudgereyem. I think we might have a bunch of these type errors which we'll have some short term pain with but will reap the benefits for years after to get it right now (will discuss also in the other issue). So thanks for fixing this, I'll say also that for simple things like this you can just submit a PR without a whole write up (unless you enjoy it :) just trying to save you some time).

Will address the caching issue elsewhere.

pudgereyem commented 3 years ago

Hey @gadicc, I agree! And yeah, I might start creating PR's without issues going forward. However, I sort of like creating issues because it gives me time to "focus on the problem" rather than "implementing a solution". It's also a good practice for documentation in its own way I believe.

Super!

gadicc commented 3 years ago

:tada: This issue has been resolved in version 1.7.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

gadicc / node-yahoo-finance2