search

Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
920 stars 167 forks source link

Async #378

Closed StorytellerCZ closed 11 months ago

StorytellerCZ commented 1 year ago

Continuation of #361

StorytellerCZ commented 1 year ago

Now we mainly need to add test suite for async.

StorytellerCZ commented 1 year ago

OK, figured out the tests, now it is just about converting the test suite to it.

StorytellerCZ commented 1 year ago

Got all the async tests running. I have removed migration tests which I don't feel need to be in the async tests as they should have been run before upgrading to Meteor 3, maybe we should mention that in the readme.

There seems to be a bug in the async version of detecting roles cycles in _addRoleToParentAsync function, which seem to break these tests. Right now I don't see what it could be as the async transition seems to have been done well. Fixing that I think could fix at least half the failing tests. @bratelefant any ideas?

bratelefant commented 1 year ago

Got all the async tests running. I have removed migration tests which I don't feel need to be in the async tests as they should have been run before upgrading to Meteor 3, maybe we should mention that in the readme.

There seems to be a bug in the async version of detecting roles cycles in _addRoleToParentAsync function, which seem to break these tests. Right now I don't see what it could be as the async transition seems to have been done well. Fixing that I think could fix at least half the failing tests. @bratelefant any ideas?

Guess I fixed this, cf #380

Sometimes it's the small things that matter ;)

jankapunkt commented 1 year ago

Yup, tests are fixed, thanks @bratelefant 🙌

@StorytellerCZ from my end it's good, should we do a final review or do you think it's fine?

StorytellerCZ commented 1 year ago

I think we might need client side tests for async as well, but I think it is fine. I will prepare a feature release once I get some time.

StorytellerCZ commented 11 months ago

@jankapunkt so the client tests were much easier than I thought, honestly I kind of find them lacking, but that is for another time. For me we can merge and release this as a new feature version. I will have to look into re-generating the docs with this.