krakenjs / cross-domain-utils

Cross Domain utilities
Apache License 2.0
131 stars 18 forks source link

stringifyDomainPattern utility unit tests and bug fixes #13

Closed Abdul-Sen closed 4 years ago

Abdul-Sen commented 4 years ago

12 . Hi, I wrote some tests for the stringifyDomainPattern utility and fixed some bugs that my unit tests came across in the function.

There were 2 minor changes made to the utility function and the reasons for those changes are listed below :

Change # 1

Initially my should stringify a single regex expression to RegExp test was failing with the following error:

Screenshot-from-2020-10-03-19-20-22

I fixed this issue by simply adding a ) in the return string to stringifyDomainPattern function in src/utils.js that seemed to be missing because of a typo which then ran my unit test successfully:

Screenshot-from-2020-10-03-19-25-28

Change # 2

I noticed that the function also accepts array of regex patterns but the return was without RegExp but still had parentheses which indicated that the RegExp function missing was a mistake. It also made for inconsistant string returns. This then caused my test should stringify an array of domain patterns to RegExp to break with the folowing output:

Screenshot-from-2020-10-03-21-36-58

I simply added RegExp to the return string and that change made my test succeed

Screenshot-from-2020-10-03-21-38-32

Please let me know if anything needs to be reverted or if there is something else you'd like me to do to meet the merge requirements.

bluepnume commented 4 years ago

Thanks for taking this up! Added one small comment.

Abdul-Sen commented 4 years ago

Hey can i close this PR and continue to add new unit tests for other utilities under the same issue but with different PRs? I apologize I am still fairly to open source contributions.

bluepnume commented 4 years ago

Hi @Abdul-Sen -- why not keep this open, and make other PRs from different branches? You can make multiple PRs to the same repo, from different branches, and keep them separate that way.

If you can fix the array-of-strings case, I'd be happy to merge this PR in 👍