search

jeff-zucker / solid-file-client

A Javascript library for creating and managing files and folders in Solid data stores
MIT License
55 stars 18 forks source link

v1.0.0 Testing #52

Closed Otto-AA closed 4 years ago

Otto-AA commented 5 years ago

An issue to discuss testing and keep track of testing for the v1.0.0 release

TODOs:

If you think that something is done, just add a comment. If something is missing, just add it.

jeff-zucker commented 5 years ago

Mentioning the storage.dump() becuase it will be useful in debugging the problem you encountered with createFolder.

jeff-zucker commented 5 years ago

OK, there was a bug in both creating containers in localStorage. I am fixing it and will commit, but wanted to give you a heads up that I probably discovered the source of the problem.

Otto-AA commented 5 years ago

There is a TODO test for SolidApi can you detail a bit the list so I can work on

Sorry, I think I forgot to answer that previously

// TODO: [Add when supported by solid-rest] test('post resolves with 201 writing to the location of an existing folder', () => resolvesWithStatus(api.post(dataFolder, getPostOptions(usedFolderName)), 201))

These tests are currently not possible to run, because solid-auth-cli behaves differently than expected. Therefore they would fail even if it is properly implemented in the solid-file-client, so I've commented them out with the TODO, so we will add it later when solid-auth-cli supports this. I've also added these to the top of this issue.

jeff-zucker commented 5 years ago

Sorry, it is taking me longer than expected to fix the solid-rest problems. None of the problems we are encountering have anything to do with solid-auth-cli, they are all in either solid-rest or solid-rest-localStorage or solid-rest-file. The trailing slash on containers is apparently confusing things and I haven't figured out yet where. I'll let you know when I commit a fix capable of addressing the TODO issues.

On Mon, Jun 17, 2019 at 4:36 AM A_A notifications@github.com wrote:

There is a TODO test for SolidApi can you detail a bit the list so I can work on

Sorry, I think I forgot to answer that previously

// TODO: [Add when supported by solid-rest] test('post resolves with 201 writing to the location of an existing folder', () => resolvesWithStatus(api.post(dataFolder, getPostOptions(usedFolderName)), 201))

These tests are currently not possible to run, because solid-auth-cli behaves differently than expected. Therefore they would fail even if it is properly implemented in the solid-file-client, so I've commented them out with the TODO, so we will add it later when solid-auth-cli supports this. I've also added these to the top of this issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jeff-zucker/solid-file-client/issues/52?email_source=notifications&email_token=AKVJCJBFATNZ3MUBSVVA3GDP25ZLVA5CNFSM4HXOKHI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX24OXY#issuecomment-502646623, or mute the thread https://github.com/notifications/unsubscribe-auth/AKVJCJEMZLJ2GKRC56XFH3TP25ZLVANCNFSM4HXOKHIQ .

jeff-zucker commented 5 years ago

All right, I mostly fixed solid-rest but also discovered that the problem with the TODO items in SolidApi.test.js are because you are trying to post with a slug that has a trailing slash. The correct response to a post with a trailing slash in the slug is fail 400. That's what NSS does and solid-rest-localStorage and solid-rest-file. Those tests would fail against NSS, they have nothing to do with solid-rest AFAIK.

You need to not create folders with api.post, only with createFolder and in that method, you need to remove the trailing slash.

jeff-zucker commented 5 years ago

Look at the first test in tests/all - it checks to see if fail 400 is returned when attempting to post with a trailing slash. It returns OK in all three environments. If you put 201 as what to expect, it will fail in all three.

jeff-zucker commented 5 years ago

I've explained the reason your methods are failing here : https://github.com/jeff-zucker/solid-file-client/issues/52#issuecomment-502402348

jeff-zucker commented 5 years ago

Oh, looks like this is all handled in @bourgeoa's patch. Should have read that first, sorry. Good work!

jeff-zucker commented 5 years ago

Are there any other outstanding issues that you believe are related to solid-rest?

jeff-zucker commented 5 years ago

I have created test/utils/getBase.js which returns the appropriate base folder (whatever is above "test-folder") depending on environment. It returns "/" for localStorage, cwd() for file, and host-from-webId/public/ for https.

It strikes me that TestFolderGenerator should use SolidApi methods. This would transparently pass them to the correct test environment as well as permitting deleteFolderRecursive (which is going to be damned hard to duplicate raw). If that fails, well then, no need to test further anyway, fix that first. Another alternative is to use solid-auth-cli because we'll need that anyway for https: and it too would transparently pass all fetches to the right environment.

Otto-AA commented 5 years ago

I have created test/utils/getBase.js which returns the appropriate base folder (whatever is above "test-folder") depending on environment. It returns "/" for localStorage, cwd() for file, and host-from-webId/public/ for https.

I've already got something similar in my local branch. Didn't push it because it failed creating the app://ls context (and probably has some other errors I don't know of yet).

async function getBaseUrl(prefix) {
    if (prefix === prefixes.file) {
        return `${prefix}${process.cwd()}/`
    }
    if (prefix === prefixes.memory) {
        return `${prefix}`
    }
    if (prefix === prefixes.https) {
        try {
            await auth.login()
            if (!process.env.TEST_BASE_URL) {
                throw new Error("Please specify the base url if you use the https prefix")
            }
            // Expects something like https://test.solid.community/private/tests/
            let base = process.env.TEST_BASE_URL + process.env.TEST_BASE_URL.endsWith('/') ? '' : '/'
            base += 'test-folder/'
            return base
        }
        catch (e) {
            console.error('Error while setting up the https base url')
            console.error(e)
            throw e
        }
    }

    throw new Error('Unsupported prefix in getBaseUrl: ' + prefix)
}

The script uses process.env.TEST_PREFIX and if it is https also process.env.TEST_BASE_URL to set up the testing environment. It defaults to app://ls now. Also it uses solid-auth-cli as a base for all testing environments. I am using TEST_BASE_URL instead of the webid because the webId doesn't need to be the same as the storage location. Also this gives the developer the choice to select private/tests/foo or something like that as base too. I am appending "test-folder" nonetheless so it doesn't accidentally overwrites other data in the pod.

I am not sure if I understood what you mean about using the SolidApi methods in the TestFolderGenerator, I'll just explain my view on it: I am not really happy with using api.deleteFolderRecursively in the setup because it makes debugging harder. If the setup fails, it will tell us to check if deleteFolderRecursively works. We will be able to see the error thrown and may conclude based on that what triggered it. But it seems a bit more tricky to debug than I'd like it to be. It just seems weird to expect a method to work which consists of 5 other methods we actually want to test. Apart from showing that something doesn't work it would be nice to see what exactly doesn't work, hence I'd like the test setup to always succeed.

Please tell me when you think solid-rest should work with the described app://ls/test-folder post creation @jeff-zucker

jeff-zucker commented 5 years ago

I was getting an error on @bougeoa's new "create file with valid slug" because postOptions was allowing bad slugs. A fix on that also cleared the first two of the three to-do tests. AFAIK the only remaining one that is a solid-rest issue is creating recursive folders. mkdir is not creating the full requested recursive tree, I need to investigate, possibly use fs.extras. At any rate, other than that, I do not know of anywhere related to our tests where solid-rest behaves wrongly. If you spot something, of course, let me know.

jeff-zucker commented 5 years ago

TEST_BASE_URL instead of the webid

Yes, cleaner that way. And developer should have to choose to run the login test, only app: should automatically run for the public but we need the other two in there to test.

I am not sure if I understood what you mean about using the SolidApi methods in the TestFolderGenerator,

Nevermind, I agree wtih not using it in the tests. But good luck erasing all the duplicate files you will have c\ reated :-)

Please tell me when you think solid-rest should work with the described app://ls/test-folder post creation

I'm not sure what you mean. It works fine for all post methods. I've been using it to create my test folders in\ all three environements. But do always do an npm install to pull in latest patches, currently at 1.0.4. If you\ find something other than the mentioned recursive folder creation that is wrong, please let me know.

jeff-zucker commented 5 years ago

Yes! I fixed the mkdir p problem and so the final to-do test in Solid-Api now passes. I am marking solid-rest as complete, though please unmark if you find something else. Version 1.0.5 of solid-rest is the one that passes all of our tests.

Otto-AA commented 5 years ago

Yes! I fixed the mkdir p problem and so the final to-do test in Solid-Api now passes. I am marking solid-rest as complete, though please unmark if you find something else.

Complete depends on your view of completeness :) If you consider SPARQL, access restrictions and websockets it is definitely not complete and likely won't ever be complete (dunno how you would do websockets for example). I think OPTIONS and PATCH requests would be good to have within a "complete" version. If there is an useful SPARQL that would be cool too. I think a way to restrict app access (ie solid-filemanager only has access to READ/WRITE but not CONTROL) would be useful for other storages than ls (ie dropbox if that's possible). I think there's a trustedApp feature in the spec somewhere, but I'm not familiar with it yet.

So from my point of view I would consider it semi-complete, especially without the OPTIONS support. And as a side note: If OPTIONS and PATCH are not supported they shouldn't be sent back by the allow header if I'm not mistaken.

Otto-AA commented 5 years ago

I think solid-rest should return a Response object created with the Response constructor. If it is just an object res instanceof Response will fail. And the response object is currently missing the url property. You can take a look here on what properties and methods would be expected.

jeff-zucker commented 5 years ago

I did not in any way mean that the package is complete or inclusive of everything it could do. I meant only that it is complete in so far as we need it to be complete to test solid-file-client. Of course there are many possibilities down the line and I had thought of Dropbox as one possibility too. You're entirely right that I shouldn't be sending the OPTIONS and PATCH tags.

But how about in terms of what we need to test solid-file client, would you say that we are there or close to there? I hope so because trying to develop two parts of the same chain at the same time is confusing.

On Mon, Jun 17, 2019 at 11:24 PM A_A notifications@github.com wrote:

Yes! I fixed the mkdir p problem and so the final to-do test in Solid-Api now passes. I am marking solid-rest as complete, though please unmark if you find something else.

Complete depends on your view of completeness :) If you consider SPARQL, access restrictions and websockets it is definitely not complete and likely won't ever be complete (dunno how you would do websockets for example). I think OPTIONS and PATCH requests would be good to have within a "complete" version. If there is an useful SPARQL that would be cool too. I think a way to restrict app access (ie solid-filemanager only has access to READ/WRITE but not CONTROL) would be useful for other storages than ls (ie dropbox if that's possible). I think there's a trustedApp feature in the spec somewhere, but I'm not familiar with it yet.

So from my point of view I would consider it semi-complete, especially without the OPTIONS support. And as a side note: If OPTIONS and PATCH are not supported they shouldn't be sent back by the allow header if I'm not mistaken.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jeff-zucker/solid-file-client/issues/52?email_source=notifications&email_token=AKVJCJAU4QUJ52OJJV2U63DP3B5QZA5CNFSM4HXOKHI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5KFUQ#issuecomment-502964946, or mute the thread https://github.com/notifications/unsubscribe-auth/AKVJCJC4RMUPWEATW47CENDP3B5QZANCNFSM4HXOKHIQ .

jeff-zucker commented 5 years ago

Yes, you're right it should and many other things too. I'm adding your valuable suggestions to my to-do eventually list,

On Tue, Jun 18, 2019 at 12:01 AM A_A notifications@github.com wrote:

I think solid-rest should return a Response object created with the Response constructor https://developer.mozilla.org/en-US/docs/Web/API/Response/Response. If it is just an object res instanceof Response will fail. And the response object is currently missing the url property. You can take a look here https://developer.mozilla.org/en-US/docs/Web/API/Response#Properties on what properties and methods would be expected.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jeff-zucker/solid-file-client/issues/52?email_source=notifications&email_token=AKVJCJBIB7TA7GMEQE2HLGLP3CB6HA5CNFSM4HXOKHI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5MTQY#issuecomment-502974915, or mute the thread https://github.com/notifications/unsubscribe-auth/AKVJCJHBD4IW3LTVDYNZMX3P3CB6HANCNFSM4HXOKHIQ .

Otto-AA commented 5 years ago

I think for solid-file-client it is nearly good enough. The OPTIONS method would be useful before releasing solid-file-client because it is also used in SolidApi.js. I think with this method it would be complete enough for solid-file-client (assuming that there are no bugs, but we will see that in the process anyway).

And the location header seems inconsistent for files and folders to me. I've made a deletion request with file:///foo/bar and for files it responded with the location file:/foo/bar.ttl and for folders with file://foo/bar (or the other way around, not sure if I remember correctly)

jeff-zucker commented 5 years ago

I'll implement some form of OPTIONS so we can at least test that the SolidApi call fetches something. I'll definitely check out the location header, let me know if you discover more details.

jeff-zucker commented 5 years ago

[things missing in solid-rest ... ] If you consider SPARQL

That would be nice, I am thinking about it. But of course, NSS doesn't really implement SPARQL - no querying, restricted subset, etc.

, access restrictions and websockets

Neither of those is in the solid rest spec, which is part of why I call it solid-rest and not solid-ldp

bourgeoa commented 5 years ago

I'm wondering if these 2 tests are correct : 

    test('post resolves with 201 writing to the location of an existing folder', () => resolvesWithStatus(api.post(postFolder.url, getPostOptions(newFolder)), 201))
    test('post resolves with 201 writing to the location of an existing file', () => resolvesWithStatus(api.post(postFolder.url, getPostOptions(newFile)), 201))

For me newFile and newFolder do not exist because of the : 

beforeEach(() => postFolder.reset({ dryRun: false }))

and should be replaced by usedFolder and usedFile. In such case they do not pass the test, with the same error : 

  ● core methods › creators › post resolves with 201 writing to the location of an existing folder

    TypeError: name.replace is not a function

      61 |
      62 |     const getPostOptions = (name) => {
    > 63 |       const slug = name.replace(/\/$/,'')
         |                         ^
      64 |       const link = name.endsWith('/') ? LINK.CONTAINER : LINK.RESOURCE
      65 |       return {
      66 |         headers: {

      at replace (tests/SolidApi.test.js:63:25)
      at Object.getPostOptions (tests/SolidApi.test.js:82:132)
jeff-zucker commented 5 years ago

I'm wondering if these 2 tests are correct :

Yes, good catch, I put NewFile in there because I didn't look far enough up to see the BeforeEach() and didn't want to create usedFile and usedFolder variables since I didn't know what Otto intended. But usedFile and usedFolder should definitely be used here.

jeff-zucker commented 5 years ago

The fix is to use usedFile,name in the call to postOptions, not usedFile. They both work with that change. I've committed the fix. All tests pass again.

Otto-AA commented 5 years ago

Yeah, that shouldn't have been that way, thanks for mentioning it @bourgeoa The reason your change didn't work is that the getPostOptions method expects a string to be passed, but usedFolder is an object. I guess we should rename newFolder to newFolderName, so the difference is more obvious from the name.

Otto-AA commented 5 years ago

@jeff-zucker I am currently struggling with the malformed url error I've mentioned yesterday. It gives errors in my current attempt to setup the testing environment. I am not sure why it works in our current version, but I think the problem is in folderUtils.processFolder (or lower). I'm encountering it when recursively deleting a folder which parses a folder to delete its contents.

Here is the output for deleteFolderContents(file:///home/aa/coding/solid/solid-file-client/test-folder/)

  deleteFolderContents file:///home/aa/coding/solid/solid-file-client/test-folder/
    folders [ { type: 'folder',
        modified: undefined,
        size: undefined,
        mtime: undefined,
        label: '',
        name: 'SolidApi',
        url: 'file:/home/aa/coding/solid/solid-file-client/test-folder/SolidApi/' } ]
    files []

I think the file:/home part causes the errors I see (not 100% sure though) Can you reproduce the error on your side?

Otto-AA commented 5 years ago

@jeff-zucker What is the goal of following line? I think there is probably an error with your catch here: https://github.com/jeff-zucker/solid-file-client/blob/otto-changes/tests/high-level-crud-response.test.js#L207

let res = await fc.readFolder(url).catch(e=>{return e.status})`

This will set res to the resolved value of readFolder if it succeeds and to e.status if it fails. The return statement inside the catch says: "The error has been handled, now act as if e.status was resolved", hence res will equal to e.status afterwards (which doesn't seem like what you expected judging from the code below it).

And in general I am no fan of functions like these, because they have pretty inconsistent return behaviour and therefore are harder to use. One needs to take a look at the function and understand when it returns what type before using it. If you would use something like resolvesWithStatus(fc.readFolder(...), 200) it is pretty clear just by the function name what it will do in which scenarios and one doesn't need to look into function implementation. With the testing environment update I will move the methods I use for this into a test-utility file so you can also use them if you want. If you prefer your way, then keep it, just wanted to mention my view on that :)

bourgeoa commented 5 years ago

Malformed folder url from folderUtils.getFolderItems : Could it be that these lines are the origin of the error

                  item.value = item.value.replace(/[/]+/g,'/');
                  item.value = item.value.replace(/https:/,'https:/');
                 .......
                  newItem.url  = item.value

They shall replace file:///home/.... with file:/home/... in the @Otto-AA example above (in the first line the escape character is missing) Could the 2 lines be deleted.

jeff-zucker commented 5 years ago

deleteFolderContents

I kept looking in solid-rest thinking it was something there. But I looked in utils/folderUtils and it was there. Near but not exactly where @bourgeoa found. Fixed it and committed, you should have all three slashes on the file urls.

On a more general note - I need to look at all of folderUtils with a finer eye when the dust settles. Probably when I switch to N3 which I'm now definitely in favor of doing.

jeff-zucker commented 5 years ago

let res = await fc.readFolder(url).catch(e=>{return e.status})`

An artfact of previous debugging to be removed.

jeff-zucker commented 5 years ago

Ok, I've gone down a bit of a rabbit hole but I believe you will be pleased with the results. I am rewriting folderUtils using N3 and optimizing as I go. I now have a getContainer method that produces a complete listing of a folder's contents. By complete I mean all files including hidden ones. Alain, I hope you like the fourth one down :-). This is output from the getContainer method. It only shows .acl and .meta if they exist.

[                                                                               
 {url:'https://jeffz.solid.community/public/Music/', type: 'Container' },       
 {url:'https://jeffz.solid.community/public/Music/.acl', type: '.acl' },        
 {url:'https://jeffz.solid.community/public/Music/.meta', type: '.meta' },      
 {url:'https://jeffz.solid.community/public/Music/.meta.acl',type: '.acl' },    
 {url:'https://jeffz.solid.community/public/Music/Ambient/',type:'Container'},  
 {url:'https://jeffz.solid.community/public/Music/lists.ttl',type:'Resource' } 
]
jeff-zucker commented 5 years ago

I've made two improvements : the list is now reversed so the top folder comes last and its contents follow it. And I changed the types of .acl and .meta to AccessControl and Metadata.

jeff-zucker commented 5 years ago

I now have a deleteContainer working based on the getContainer so that it does a "complete" delete of folder contents including .acl, .meta, and .met.acl.

BTW, interesting discovery : the .meta of a container e.g. /public/foo/.meta is listed in its parent's folder i.e. a listing of /public/ turns up /public/foo/.meta so it's a bit unclear which folder it is "contained in", perhaps both.

Otto-AA commented 5 years ago

That sounds nice @jeff-zucker

I am a bit confused for now where this getContainer method exists (is it instead of processFolder or readFolder or in addition to both or ...?). But I am looking forward to see it :)

Otto-AA commented 5 years ago

Fixed it and committed, you should have all three slashes on the file urls.

Works like a charm now :+1:

On a more general note - I need to look at all of folderUtils with a finer eye when the dust settles. Probably when I switch to N3 which I'm now definitely in favor of doing.

Probably we should remove the tick at "Finish tests for folderUtils.js" then? If we aren't sure if everything works fine in there it would be a good reminder.

jeff-zucker commented 5 years ago

It will essentially be a replacement for ReadFolder expanded to include .acl and .meta. It will probably replace the entire chain - text2graph, getFolderItems, processFolder, etc. FolderUtils will need to be a class and get it's auth object when initialized because it needs to do various fetches.

On Thu, Jun 20, 2019 at 3:38 AM A_A notifications@github.com wrote:

That sounds nice @jeff-zucker https://github.com/jeff-zucker

I am a bit confused for now where this getContainer method exists (is it instead of processFolder or readFolder or in addition to both or ...?). But I am looking forward to see it :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jeff-zucker/solid-file-client/issues/52?email_source=notifications&email_token=AKVJCJAEMA4IY7ESVYFOJGTP3NMZDA5CNFSM4HXOKHI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYFA3FI#issuecomment-503975317, or mute the thread https://github.com/notifications/unsubscribe-auth/AKVJCJHAEPVPHDY54ATEJWDP3NMZDANCNFSM4HXOKHIQ .

Otto-AA commented 5 years ago

It will essentially be a replacement for ReadFolder expanded to include .acl and .meta. It will probably replace the entire chain - text2graph, getFolderItems, processFolder, etc. FolderUtils will need to be a class and get it's auth object when initialized because it needs to do various fetches.

I think we would get more value out of it if we add some methods to folderUtils for processing acl and meta files and then add a method in SolidApi which utilizes folderUtils. I guess that when we use a ReadFolder class the methods inside it won't be available for others to use which would be useful from my point of view.

My reasoning behind this is, that when working with .acl files it would be handy for the user to already have some methods they can use (e.g. SolidApi.fetchAclForItem(url) or SolidApi.get(folderUtils.getAclUrlFromResponse(response))). If we hide this inside a ReadFolder class it seems harder to give the user the ability to also use those methods. The other reason is, that we ourselves may also want to use those acl utility functions (e.g. for copying a file with its permissions). I guess that depends on how exactly we implement those, but I would leave the option open.

Therefore I would advocate for defining the utility functions used for this in folderUtils (or xyzUtils) and also exposing it to the user somehow (e.g. SolidFileClient.utils.getAclUrlFromResponse(...)). Would you share that view, or have a different perspective on it? Or did I completely misunderstand you once again...?

jeff-zucker commented 5 years ago

I think we would get more value out of it if we add some methods to folderUtils for processing acl and meta files and then add a method in SolidApi which utilizes folderUtils.

Sure, that's what I plan. But do I initialize folderUtils with auth in SolidFileClient and pass that to SolidApi, or does SolidApi get full auth and use it to initalize folderUtils?

I guess that when we use a ReadFolder class the methods inside it won't be available for others to use which would be useful from my point of view.

Yes exactly, getLinks() etc.

If we hide this inside a ReadFolder class

No, that was never my intention. It will be folderUtils class which will have a readFolder method.

Would you share that view,

Yes, that is how I have been thinking.

The folderUtils.getLinks method now has this interface:

 /**                                                                           
 * getLinks(itemUrl)                                                          
 *                                                                            
 * returns an array of records related to an item (resource or container)     
 *   0   : the item itself                                                    
 *   1-3 : the .acl, .meta, and .meta.acl for the item if they exist          
 * each record includes these fields                                          
 *   url                                                                      
 *   type (one of Container, Resource, AccessControl, or Metadata)            
 *   content-type (text/turtle, etc.)                                         
 */
bourgeoa commented 5 years ago

@Otto-AA I made the tests in with your test branch, here is the result for : 


 - for `file` and `ls` there is one more test fail 39 in lieu of 38.

Is it what you expected ?
jeff-zucker commented 5 years ago

I probably mispoke to say the method I am working on is a replacement for readFolder, it is a replacement for processFolder. One benefit of the new way is that instead of the guessing filetype crap, it gets the filetype straight from the header. Another is that it reads whatever triples the container had available so instead of just stat and mtime, it will pick up anything stored in the container or its .meta.

Otto-AA commented 5 years ago

Sure, that's what I plan. But do I initialize folderUtils with auth in SolidFileClient and pass that to SolidApi, or does SolidApi get full auth and use it to initalize folderUtils?

Why do you want to put those methods into folderUtils rather than the SolidApi if they need a fetch? And do you really need auth or would fetch be sufficient?

The folderUtils.getLinks method now has this interface:

Imo an object would be more intuitive, why did you choose an array instead?

Otto-AA commented 5 years ago

@bourgeoa The test-environment branch is for debugging the 401 error I've mentioned in the PR. Most of the tests won't work in it because I've changed things to debug this error (I get the same test results if I run it like you do).

It is intended to be used only on testSetup.test.js (TEST_PREFIX=https:// TEST_BASE_URL=https://bourgeoa.solid.community/public/tests/ npx jest tests/testSetup.test.js). It is only about getting the test setup right. This test already fails in the beforeAll method on line 9, because it can't create the setup-test folder (The server responds with 401 and "Invalid PoP token signature").

The one test inside testSetup.test.js actually won't work either, but that doesn't matter. Only getting the setup working there is what the branch is for :)

bourgeoa commented 5 years ago

Thanks, I misunderstood everything

jeff-zucker commented 5 years ago

Why do you want to put those methods into folderUtils rather than the SolidApi if they need a fetch?

Doesn't matter in the least, that's where have them now, if you want them in SolidApi, that's fine.

And do you really need auth or would fetch be sufficient?

Probably, but I'll wait for the results of you debugging that in the testConfig.

an object would be more intuitive, why did you choose an array instead?

More intuitive for whom? I find your code with a zillion objects very hard to navigate. But my reasoning is that the main use of getLinks is to either copy or delete links (for which an iterable array is useful) or to add the array to the array of files and folder that processFolder creates. I'm not against turning it into an object if you really prefer.

Otto-AA commented 5 years ago

@bourgeoa No problem. I probably wasn't explaining it clear enough

Otto-AA commented 5 years ago

More intuitive for whom? I find your code with a zillion objects very hard to navigate. But my reasoning is that the main use of getLinks is to either copy or delete links (for which an iterable array is useful) or to add the array to the array of files and folder that processFolder creates. I'm not against turning it into an object if you really prefer.

const links = await getLinks(url)
api.delete(links[1])
api.delete(links.acl)

I mainly meant readable. But also more intuitive for people using IDEs which understand JSDoc (e.g. typing "links." would popup a menu with "acl, meta, meta-acl" in that case). I don't think that using an array would be a good idea because of this, and also it would need to contain undefined values if a link is not existing (which makes a if (link) check required when looping).

I find your code with a zillion objects very hard to navigate.

If you have recommendations on how I could make it better, I am open adjustments. I haven't worked in many collaborative coding projects yet, so it may be not so easily understandable. I guess the reason for the many objects is that I try to make things reusable as much as possible, maybe I am overshooting it a bit sometimes.

jeff-zucker commented 5 years ago

[the getLinks array ...] would need to contain undefined values if a link is not existing (which makes a if (link) check required when looping).

No, that is not how it works. getLinks only adds an .acl to the array if there is a .acl. So a file with no .acl or .meta is an array of length 1 and one with a .acl and a .meta and a .meta is an array of length 4. This way we end up with a big array which gets put in the body.files and body.folders of processFolder and listings of folders will include the .acl files.

It's possible that my use of this internally should be different from the public interface. But OTOH, I am totally unfamiliar with JSDoc so if you say it's important for that, I'm fine with using objects. I need to get it working first, then I can refactor as needed.

If you have recommendations on how I could make it better

Oh no, I didn't mean your code wasn't good. I meant I find the style hard for me to follow in your test suites stuff. That doesn't mean you should change it. Just pointing out that what is intuitive for you may not be for me, No criticism involved at all.

Otto-AA commented 5 years ago

No, that is not how it works. getLinks only adds an .acl to the array if there is a .acl. So a file with no .acl or .meta is an array of length 1 and one with a .acl and a .meta and a .meta is an array of length 4. This way we end up with a big array which gets put in the body.files and body.folders of processFolder and listings of folders will include the .acl files.

Oh, I thought that they had fixed position in the array (e.g. .acl always at index 1) in your way. I'd still change it to an object in the future, but we can do that later.

jeff-zucker commented 5 years ago

The only position that is fixed is the first - it is the resource itself, the others are added only if they exist.

Otto-AA commented 5 years ago

I would start adding basic logging now with the debug module. It works in node and browsers and provides the ability to log to different channels. The channels can be enabled separately, so for instance I could only enable "solid-file-client:fetch" or everything from our package with "solid-file-client:*". Something I am not entirely happy about is, that it doesn't has a log-level setting (e.g. debug/info/error/...), but I guess using different channels is sufficient. So I think it would be a good choice. Are you ok with this logger, or would you prefer another one?

For now I would only add a "solid-file-client:fetch" channel which outputs all requests and their response status code, for instance 200 - GET https://example.org/file.ext. This seems useful for debugging as we would quickly know which requests fail, so I'd like to do this now. Later on we can add logging for other methods too.