Food-Static-Data / sd

Food static data wrapper
GNU General Public License v3.0
9 stars 14 forks source link

json-schema modification [addon] #423

Closed atherdon closed 5 years ago

atherdon commented 5 years ago

1) Move json schemes into the separated variable outside of describe block. it will make tests looks less complicated 2) move out example into the separated file, move out scheme into the separated file - maybe it should be one file with 2 exported variables... 3) configure rollup bundler, so with build -> we copy only json files from data folder at build, and skip tests/schemas/examples...

22

atherdon commented 5 years ago

yes please start with second part of task, but please everybody be aware and sync your fork with our repository - because i'll merge a lot of new changes in 1 hour or so

AndrewSerra commented 5 years ago

@siddharthp538 Still not totally sure. What I understood is having a file that is similar to files.js for both examples and schemas, is that right?

siddharthp538 commented 5 years ago

@siddharthp538 Still not totally sure. What I understood is having a file that is similar to files.js for both examples and schemas, is that right?

have a look at tests/dashexamples.js something like that.

siddharthp538 commented 5 years ago

@AndrewSerra created tests/example/ we'll add all the files there. tell me which files you'll are willing to take. I have already did option 2 for few files.

atherdon commented 5 years ago

example is very unclear name. I.e "example of what?" or "if there examples, why we have schemas as well?"

siddharthp538 commented 5 years ago

example is very unclear name. I.e "example of what?" or "if there examples, why we have schemas as well?"

i meant examples of all the schema's

AndrewSerra commented 5 years ago

@siddharthp538 got it. I'm assuming you went alphabetically, I can complete from gallon.test.js to quart.test.js?

siddharthp538 commented 5 years ago

works for me @AndrewSerra

AndrewSerra commented 5 years ago

I have made a pull request for the files starting from gallon.test.js to quart.test.js @siddharthp538 just to let you know

AndrewSerra commented 5 years ago

I have made a pull request for the files starting from gallon.test.js to quart.test.js @siddharthp538 just to let you know

*when you list them alphabetically

siddharthp538 commented 5 years ago

I have made a pull request for the files starting from gallon.test.js to quart.test.js @siddharthp538 just to let you know

*when you list them alphabetically

cool, i'll do the remaining files and then we'll start with option 3.

atherdon commented 5 years ago

i think we also need tests that will prevent errors of not finding examples/schema files

siddharthp538 commented 5 years ago

i think we also need tests that will prevent errors of not finding examples/schema files

could not get you?

atherdon commented 5 years ago

?

siddharthp538 commented 5 years ago

i think we also need tests that will prevent errors of not finding examples/schema files

can u elaborate a little?

atherdon commented 5 years ago

Sure, imagine this situation - i'll delete whole folder that contains examples/schemas. without tests it will just crash. but tests will show use that something is wrong here

this is actually a similar test case that we have with out json files

atherdon commented 5 years ago

@elnur004 you can jump into this task with very simple improvement. you can go through files that we have and update this line

const schema = require('./examples/users').schema
const example = require('./examples/users').example

with

const { schema, example } = require('./examples/users')
AndrewSerra commented 5 years ago

Sure, imagine this situation - i'll delete whole folder that contains examples/schemas. without tests it will just crash. but tests will show use that something is wrong here this is actually a similar test case that we have with out json files

@atherdon so you mean adding extra test cases that check all files are there? or a simple try/catch block?

atherdon commented 5 years ago

i like both! Do both! :) extra cases and try/catch is a good thing to implement here

AndrewSerra commented 5 years ago

Nice! I can work on that or should I continue with step three? If I'll be working on error handling, for the try/catch should it just be a try { require('./example/a_file.js') } catch (e) { console.log(e) } block that writes to console or something else? For the test cases can they be a file that has a describe block containing all it and expect for all the necessary files?

atherdon commented 5 years ago

we don' ready for step 3 yet. about try/catch - show me code changes at pull requests - its better for my understanding

AndrewSerra commented 5 years ago

@atherdon should I do one and make a pull request so you could see? after that, I can continue with the rest and add them all to the same pull request

atherdon commented 5 years ago

@AndrewSerra looks like your changes break our builds. error log

 /home/travis/build/GroceriStar/sd/tests/testFilesCheck/exampleFiles.test.js:3:1: 'expect' is not defined.
  /home/travis/build/GroceriStar/sd/tests/testFilesCheck/exampleFiles.test.js:5:1: 'describe' is not defined.
  /home/travis/build/GroceriStar/sd/tests/testFilesCheck/exampleFiles.test.js:6:3: 'it' is not defined.
  /home/travis/build/GroceriStar/sd/tests/testFilesCheck/exampleFiles.test.js:7:5: 'expect' is not defined.
siddharthp538 commented 5 years ago

@atherdon I won't be available this week. Sorry, have to complete my college project.

atherdon commented 5 years ago

@siddharthp538 no problem at all - take your time - no rush here

atherdon commented 5 years ago

Andrew, I fixed that issue

AndrewSerra commented 5 years ago

@atherdon what was causing it to be not defined? I checked all other files and couldn't figure it out

AndrewSerra commented 5 years ago

Do we have anything left for step 2?

atherdon commented 5 years ago

i don't know - you tell me. Do you think we cover all json files? Are you really really sure? because i'm always unsure

AndrewSerra commented 5 years ago

I think we are done. The last part I have done checks all the files under the examples/ directory, so all the schemas and examples that are used in the tests/ directory.

atherdon commented 5 years ago

will you bet on that that we're done? :)

AndrewSerra commented 5 years ago

will you bet on that that we're done? :)

nope :)

atherdon commented 5 years ago

me too - then spend some time and try to answer to the question - what files were covered and what is not. i mean we can also create some methods as well - that can help us or some of our team members to answer to that question in future as well

siddharthp538 commented 5 years ago

can u assign a new task? @atherdon

atherdon commented 5 years ago

yes i can but for me this task is not finished yet

atherdon commented 5 years ago

try to find static files, that we have in this repo - and they were not covered yet with json schema test

AndrewSerra commented 5 years ago

@atherdon do you mean any files that we have missed to create as \<filename>.test.js

AndrewSerra commented 5 years ago

Also under tests/projects/ directory, I don't know if there were typos creating the file names, but some of the tests under the project folders have names such as "loopback.teszt.js" Just wanted to ask because it may be a typo even though they are all commented out except for graphQL at the moment.

atherdon commented 5 years ago

this is my "work" - we didn't finish test coverage for that files and i just rename them, in order to remove them from testing process. You can create a separated task/github issue - specify each file and some new teammate will be able to deal with it

siddharthp538 commented 5 years ago

@AndrewSerra can u pls explain me what is done and what is left?

atherdon commented 5 years ago

@vadim9999 grab it here as well. it mostly done, but not finished

AndrewSerra commented 5 years ago

@siddharthp538 sorry for the late response, I'm having quite a rough week. So what I have completed is a try/catch block for the test.js files so the test does not crash if for some reason a file is missing and we could not import the schema and example. The second thing I have done is the tests/testFilesCheck/ directory. The two files under that directory get all the file names under tests/examples/ and export an object. Then the exampleFiles.test.js files contain manually written tests to check if the key in the object is undefined or not.

For what needs to be done: as far as I understand we have to find static files where we have missed to create tests.

AndrewSerra commented 5 years ago

@atherdon I found a couple more, do we need tests for Derivation_Code_Description, DietAndHealthLabel, Product, and ServingSize? These are under src/data/ and they all have csv files except for DietAndHealthLabel.

atherdon commented 5 years ago

yes we need it. i think when it will be done - we'll be able to finish this big topic with tasks and focus on the next things

AndrewSerra commented 5 years ago

I'll start with Derivation_Code_Description then. Also, I will add them under food composition in files.js since the json file is under generator/ they can all be together and easier to see when the paths change later

AndrewSerra commented 5 years ago

If anyone has not done DietAndHealthLabel doing it now then openning a pull request

AndrewSerra commented 5 years ago

@atherdon I don't know if I understood it right but in some test files, there are imports similar to this

const { typesFile } = require('@files')
const types = require(typesFile)

and I found similar ones in a couple of files which return undefined for the one coming from ./src/files.js because it is not present in the file. The second line returns an empty object. Is this supposed to be like this?

Also, another thing is that I am completing the final two tests which are Product and ServingSize. Since it is similar to Nutrition, I went through that issue and saw that the readAllFiles(path) function which returns all the objects stored in all the json files in the directory. Do we need all the content?

atherdon commented 5 years ago

@vadim9999 can you advice @AndrewSerra here?

from my side - open PR and we'll take a look on it :)

AndrewSerra commented 5 years ago

I have written the example file and the test works at the moment, the problem is the json path, that we check as the first test in every file. for example this piece of code:

describe('foodComposition data files returns a path', () => {
  it('these tests prevent any issues and problems, also to break the structure of foodComposition', () => {
    expect(foodComposition).not.toBe('')
  })
})

I could still use path resolver where I only check the path of one json file, since all of them are in the same directory. In that case I can add it to src/files.js

AndrewSerra commented 5 years ago

I can open a pull request before I open a pull request, should I do what I have just mentioned in the previous comment?