Thread: [Semanticscuttle-devel] Added branch for configurable privacy ....
Brought to you by:
cweiske
From: Brett D. <bs...@fr...> - 2011-03-02 05:12:55
|
Hi, I've made changes for this feature and have added a test to BookmarkTest.php. This is the first time I've pushed a new branch using git, so I'm not sure if I've done it right. Please have a look at the configurable-privacy branch and let me know if this looks okay. Thanks. Rgrds, Brett |
From: Christian W. <cw...@cw...> - 2011-03-07 13:00:26
Attachments:
signature.asc
|
Hi Brett, > I've made changes for this feature and have added a test to > BookmarkTest.php. > > This is the first time I've pushed a new branch using git, so I'm not > sure if I've done it right. Please have a look at the > configurable-privacy branch and let me know if this looks okay. No, it's not ok. You created a completely fresh branch starting from nothing, but normally you start from an existing branch - i.e. master. $ git clone ... $ git checkout master -> branch you want to base from $ git checkout -b my-new-branch -> create new branch $ git checkout my-new-branch -> switch to new branch Now you do your changes and commit. Currently, I cannot just "diff" the changes to see what you changed. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Brett D. <bs...@fr...> - 2011-03-10 23:44:23
|
Hello, Christian. My apologies for the mix-up. I think I've corrected this by pushing to the master at origin. However, now the configurable-privacy branch shows two parents. Rgrds, Brett On Mon, 7 Mar 2011 14:00:02 +0100, Christian Weiske <cw...@cw...> wrote: > Hi Brett, > > >> I've made changes for this feature and have added a test to >> BookmarkTest.php. >> >> This is the first time I've pushed a new branch using git, so I'm not >> sure if I've done it right. Please have a look at the >> configurable-privacy branch and let me know if this looks okay. > > No, it's not ok. You created a completely fresh branch starting from > nothing, but normally you start from an existing branch - i.e. master. > > $ git clone ... > $ git checkout master > -> branch you want to base from > $ git checkout -b my-new-branch > -> create new branch > $ git checkout my-new-branch > -> switch to new branch > > Now you do your changes and commit. > Currently, I cannot just "diff" the changes to see what you changed. |
From: Christian W. <cw...@cw...> - 2011-03-14 07:47:13
Attachments:
signature.asc
|
Hello Brett, > My apologies for the mix-up. I think I've corrected this by pushing to > the master at origin. However, now the configurable-privacy branch > shows two parents. No, not really. I had to do the diff manually since you also seemed to change the line breaks in all the files. Did you notice you have a sc subdirectory? You also added files like Thumbs.db that should never be checked in the repository. About the code itself: - You do not need to declare "$defaults = array()" in config.php.dist since config.default.php is included before config.php - so redeclaring the array would actually erase the default settings for privacy. - Why do you suddenly display "public" for public bookmarks? Since public is standard, there is no need to display the text. The bookmark status is only shown for non-public bookmarks which helps to unclutter the bookmark list display in my eyes. - Your unit test is flawed since it does not test the effects of a privacy config setting at all - you set the variable and use the same variable as parameter to the method you are calling. The correct way is to set the privacy variable permanently in a config and call the scripts you changed (i.e. posts_add.php, edit.php) with a set of POST parameters via HTTP. You can then observe if the added bookmarks have the configured status. Setting the varible permanently is not easy here since you do a HTTP call, but I will add that to the master branch quickly. Always remember that for the unit tests to be sufficient they need to fail when I remove or change a line in your SemanticScuttle source code changes. Currently, I can revert the changes in www/bookmark.php and all other files without the tests to fail. Apart from that, the code seems to do what it should. About the branch: Since the configurable-privacy branch is totally broken, we should just delete it and start afresh from master and apply your changes to it. Shall I do that for you? -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Brett D. <bs...@fr...> - 2011-03-15 02:12:19
|
Hello, Christian. Thanks for the feedback/review. I will work to make corrections and emend my changes over the next few days. Given my as-yet limited experience with git, perhaps it would be best for you to delete the branch. Thanks. Rgrds, Brett On Mon, 14 Mar 2011 08:33:56 +0100, Christian Weiske <cw...@cw...> wrote: > Hello Brett, > > >> My apologies for the mix-up. I think I've corrected this by pushing to >> the master at origin. However, now the configurable-privacy branch >> shows two parents. > No, not really. I had to do the diff manually since you also seemed to > change the line breaks in all the files. > > Did you notice you have a sc subdirectory? You also added files > like Thumbs.db that should never be checked in the repository. > > About the code itself: > - You do not need to declare "$defaults = array()" in config.php.dist > since config.default.php is included before config.php - so > redeclaring the array would actually erase the default settings for > privacy. > - Why do you suddenly display "public" for public bookmarks? Since > public is standard, there is no need to display the text. The > bookmark status is only shown for non-public bookmarks which helps to > unclutter the bookmark list display in my eyes. > - Your unit test is flawed since it does not test the effects of a > privacy config setting at all - you set the variable and use the same > variable as parameter to the method you are calling. > The correct way is to set the privacy variable permanently in a > config and call the scripts you changed (i.e. posts_add.php, edit.php) > with a set of POST parameters via HTTP. You can then observe if the > added bookmarks have the configured status. Setting the varible > permanently is not easy here since you do a HTTP call, but I will add > that to the master branch quickly. > Always remember that for the unit tests to be sufficient they need to > fail when I remove or change a line in your SemanticScuttle source > code changes. Currently, I can revert the changes in www/bookmark.php > and all other files without the tests to fail. > > Apart from that, the code seems to do what it should. > > About the branch: > Since the configurable-privacy branch is totally broken, we should just > delete it and start afresh from master and apply your changes to it. > Shall I do that for you? |
From: Christian W. <cw...@cw...> - 2011-03-15 07:38:33
Attachments:
signature.asc
|
Hello Brett, > Thanks for the feedback/review. I will work to make corrections and > amend my changes over the next few days. Great. > Given my as-yet limited experience with git, perhaps it would be best > for you to delete the branch. I deleted the configurable-privacy branch and created a new one for you, configurable-privacy2. I also applied your changes to it as a commit from you. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Brett D. <bs...@fr...> - 2011-03-22 03:42:50
|
Hello, Christian. I've written a unit test for posts_add.php and will write additional asserts for the other affected files this week. I think I will need to create a bookmark file with data for testing import.php and importNetscape.php. I assume I should just place this file in the tests directory, correct? Rgrds, Brett On Tue, 15 Mar 2011 08:38:21 +0100, Christian Weiske <cw...@cw...> wrote: > Hello Brett, > > >> Thanks for the feedback/review. I will work to make corrections and >> amend my changes over the next few days. > Great. > >> Given my as-yet limited experience with git, perhaps it would be best >> for you to delete the branch. > I deleted the configurable-privacy branch and created a new one for > you, configurable-privacy2. I also applied your changes to it as a > commit from you. |
From: Christian W. <cw...@cw...> - 2011-03-22 05:58:10
Attachments:
signature.asc
|
Hello Brett, > I've written a unit test for posts_add.php and will write additional > asserts for the other affected files this week. > > I think I will need to create a bookmark file with data for testing > import.php and importNetscape.php. I assume I should just place this > file in the tests directory, correct? Yep. Although I begin to think that we should create a data/ directory inside tests/ and put the file in there so they don't clutter up the tests dir itself. Please prefix the file names with the test class name, i.e. "UserTest-netscape.html". -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Brett D. <bs...@fr...> - 2011-03-27 03:49:37
|
Hello, Christian. I have completed my unit tests and will likely check in my changes Tuesday. I have some concerns about the fact that there were Windows carriage returns in my last check-in. I am working on a Windows environment--I will eventually set up a Linux environment, but not for a few weeks at least--but I've been editing files with vi on Cygwin and MKS Toolkit, so there should not have been any Windows carriage returns. Any tips/advice you have concerning this would be appreciated. Here are a few things I noticed when testing import.php and importNetscape.php: 1. import.php points the user to http://del.icio.us/api/posts/all to save bookmarks to an XML file. This is evidently not a good option any longer for "new" del.icio.us users who are using a Yahoo! ID to access their bookmarks. A better option for these users is to go to the Delicious XML Exporter at http://deliciousxml.com/. Otherwise, they must use http://api.del.icio.us/v2/posts/all with open authorization, and that complicates the process somewhat. 2. I think I may have found a bug in importNetscape.php, which ignores the last bookmark in the specified HTML file. I think this is occurring in the regular expression given as the first parameter of the preg_match_all function call. Consequently, my test data contains a last entry that I have assumed will be ignored. The test will need to be updated if this behavior in importNetscape.php changes. Also, I will be creating a data directory below the tests directory to keep the test data for import.php and importNetscape.php. Rgrds, Brett On Tue, 22 Mar 2011 06:57:55 +0100, Christian Weiske <cw...@cw...> wrote: > Hello Brett, > > >> I've written a unit test for posts_add.php and will write additional >> asserts for the other affected files this week. >> >> I think I will need to create a bookmark file with data for testing >> import.php and importNetscape.php. I assume I should just place this >> file in the tests directory, correct? > Yep. Although I begin to think that we should create a data/ directory > inside tests/ and put the file in there so they don't clutter up the > tests dir itself. Please prefix the file names with the test class > name, i.e. "UserTest-netscape.html". |
From: Christian W. <cw...@cw...> - 2011-04-06 07:55:14
Attachments:
signature.asc
|
Hello Brett, > I have some concerns about the fact that there were Windows > carriage returns in my last check-in. I am working on a Windows > environment--I will eventually set up a Linux environment, but not > for a few weeks at least--but I've been editing files with vi on > Cygwin and MKS Toolkit, so there should not have been any Windows > carriage returns. No problem. Someone noticed it, now it's fixed, all is fine :) > Any tips/advice you have concerning this would be > appreciated. Sorry, I've never worked with cygwin or so. I know that many editors/IDEs on windows are able to save files with unix newlines only, i.e. Notepad++ and all the PHP IDEs. > Here are a few things I noticed when testing import.php and > importNetscape.php: > 1. import.php points the user to http://del.icio.us/api/posts/all to > save bookmarks to an XML file. This is evidently not a good option any > longer for "new" del.icio.us users who are using a Yahoo! ID to access > their bookmarks. A better option for these users is to go to the > Delicious XML Exporter at http://deliciousxml.com/. Otherwise, they > must use http://api.del.icio.us/v2/posts/all with open authorization, > and that complicates the process somewhat. Yep, we should change that. > 2. I think I may have found a bug in importNetscape.php, which ignores > the last bookmark in the specified HTML file. I think this is > occurring in the regular expression given as the first parameter of > the preg_match_all function call. Consequently, my test data contains > a last entry that I have assumed will be ignored. The test will need > to be updated if this behavior in importNetscape.php changes. Ok, I'll see if I can fix that. About your tests: - You assume that the default privacy is configured to the default one, but that's something you cannot assume. If you do such assumptions, at least do a assertion on your assumed variable so the test fails with an explanatory message. - You assume a default privacy setting since there was no way to configure the privacy permanently for http requests - up to now! In the jquery branch, I had the same problem and solved it; the testbaseapi class has a way to set default configuration values. - You had one big method in BookmarksTest that checked all the different api methods. In general, keeping test methods small is a really good idea since in case of a failure, you can narrow down the problematic code line much faster. The rule of thumb is to test one thing in a test (but I don't want only one assertion in one test method, that'd be overkill). - I began to split up the big test method and moved the api/posts_add test to their corresponding Api test class. By doing that I could utilize the TestBaseApi methods that prepare authorized HTTP requests automatically for you if you wish that. Do the same for the rest of the tests in there. - In tests, always use example.org as hostname, or example.com and example.net (of course, you can use foo.example.org, example.org/foobar and such). - You assume that new bookmarks always get ID 1 in your tests. That might not always be the case, especially with database systems other than mysql. Better than fetching bookmark ID 1 is to fetch all bookmarks of the user with the ID you just created which keeps you on the safe side. - Assuming http://localhost/ as URL fails for my setup since I have "http://bm.bogo/" as URL under which my dev instance of SC is available. TestBaseApi takes care of this. I cherry-picked the two relevant changes to testbaseapi into your branch. You still need to merge master, though. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Brett D. <bs...@fr...> - 2011-04-09 00:48:10
|
Hello, Christian. Thanks for the helpful suggestions. I will work to improve the tests this weekend. Rgrds, Brett On Wed, 6 Apr 2011 09:55:01 +0200, Christian Weiske <cw...@cw...> wrote: > Hello Brett, > > >> I have some concerns about the fact that there were Windows >> carriage returns in my last check-in. I am working on a Windows >> environment--I will eventually set up a Linux environment, but not >> for a few weeks at least--but I've been editing files with vi on >> Cygwin and MKS Toolkit, so there should not have been any Windows >> carriage returns. > No problem. Someone noticed it, now it's fixed, all is fine :) > >> Any tips/advice you have concerning this would be >> appreciated. > Sorry, I've never worked with cygwin or so. I know that many > editors/IDEs on windows are able to save files with unix newlines only, > i.e. Notepad++ and all the PHP IDEs. > > >> Here are a few things I noticed when testing import.php and >> importNetscape.php: >> 1. import.php points the user to http://del.icio.us/api/posts/all to >> save bookmarks to an XML file. This is evidently not a good option any >> longer for "new" del.icio.us users who are using a Yahoo! ID to access >> their bookmarks. A better option for these users is to go to the >> Delicious XML Exporter at http://deliciousxml.com/. Otherwise, they >> must use http://api.del.icio.us/v2/posts/all with open authorization, >> and that complicates the process somewhat. > Yep, we should change that. > >> 2. I think I may have found a bug in importNetscape.php, which ignores >> the last bookmark in the specified HTML file. I think this is >> occurring in the regular expression given as the first parameter of >> the preg_match_all function call. Consequently, my test data contains >> a last entry that I have assumed will be ignored. The test will need >> to be updated if this behavior in importNetscape.php changes. > Ok, I'll see if I can fix that. > > > About your tests: > - You assume that the default privacy is configured to the default one, > but that's something you cannot assume. If you do such assumptions, > at least do a assertion on your assumed variable so the test fails > with an explanatory message. > - You assume a default privacy setting since there was no way to > configure the privacy permanently for http requests - up to now! In > the jquery branch, I had the same problem and solved it; the > testbaseapi class has a way to set default configuration values. > - You had one big method in BookmarksTest that checked all the > different api methods. In general, keeping test methods small is a > really good idea since in case of a failure, you can narrow down the > problematic code line much faster. The rule of thumb is to test one > thing in a test (but I don't want only one assertion in one test > method, that'd be overkill). > - I began to split up the big test method and > moved the api/posts_add test to their corresponding Api test > class. By doing that I could utilize the TestBaseApi methods that > prepare authorized HTTP requests automatically for you if you wish > that. Do the same for the rest of the tests in there. > - In tests, always use example.org as hostname, or example.com and > example.net (of course, you can use foo.example.org, > example.org/foobar and such). > - You assume that new bookmarks always get ID 1 in your tests. That > might not always be the case, especially with database systems other > than mysql. Better than fetching bookmark ID 1 is to fetch all > bookmarks of the user with the ID you just created which keeps you on > the safe side. > - Assuming http://localhost/ as URL fails for my setup since I have > "http://bm.bogo/" as URL under which my dev instance of SC is > available. TestBaseApi takes care of this. > > I cherry-picked the two relevant changes to testbaseapi into your > branch. You still need to merge master, though. |
From: Brett D. <bs...@fr...> - 2011-04-14 04:42:23
|
Hello, Christian. I'm having trouble with a few items in PostsAddTest.php. I tried to run this test in PHPUnit and kept getting 404 responses from the Web server. Then I noticed line 36: protected $urlPart = 'api/posts/add'; I thought maybe this should be 'api/posts_add' and changed it. That eliminated the 404s, and so I thought it might be a mistake, but I was also unable to get the tests to run without explicitly specifying $urlSuffix because it was not getting set in TestBaseApi.php. I assume that I'm missing a bunch of setup tasks because these tests should not be able to pass as I first attempted to run them in PHPUnit. Or maybe I should be running the tests all at once? Rgrds, Brett > On Wed, 6 Apr 2011 09:55:01 +0200, Christian Weiske > <cw...@cw...> wrote: >> Hello Brett, >> >> About your tests: >> - You assume that the default privacy is configured to the default one, >> but that's something you cannot assume. If you do such assumptions, >> at least do a assertion on your assumed variable so the test fails >> with an explanatory message. >> - You assume a default privacy setting since there was no way to >> configure the privacy permanently for http requests - up to now! In >> the jquery branch, I had the same problem and solved it; the >> testbaseapi class has a way to set default configuration values. >> - You had one big method in BookmarksTest that checked all the >> different api methods. In general, keeping test methods small is a >> really good idea since in case of a failure, you can narrow down the >> problematic code line much faster. The rule of thumb is to test one >> thing in a test (but I don't want only one assertion in one test >> method, that'd be overkill). >> - I began to split up the big test method and >> moved the api/posts_add test to their corresponding Api test >> class. By doing that I could utilize the TestBaseApi methods that >> prepare authorized HTTP requests automatically for you if you wish >> that. Do the same for the rest of the tests in there. >> - In tests, always use example.org as hostname, or example.com and >> example.net (of course, you can use foo.example.org, >> example.org/foobar and such). >> - You assume that new bookmarks always get ID 1 in your tests. That >> might not always be the case, especially with database systems other >> than mysql. Better than fetching bookmark ID 1 is to fetch all >> bookmarks of the user with the ID you just created which keeps you on >> the safe side. >> - Assuming http://localhost/ as URL fails for my setup since I have >> "http://bm.bogo/" as URL under which my dev instance of SC is >> available. TestBaseApi takes care of this. >> |
From: Christian W. <cw...@cw...> - 2011-04-14 17:46:34
Attachments:
signature.asc
|
Hello Brett, > I'm having trouble with a few items in PostsAddTest.php. I tried to > run this test in PHPUnit and kept getting 404 responses from the Web > server. Then I noticed line 36: > > protected $urlPart = 'api/posts/add'; > > I thought maybe this should be 'api/posts_add' and changed it. That > eliminated the 404s, and so I thought it might be a mistake I'll check that - we have URL rewriting rules that may affect that. >, but I was > also unable to get the tests to run without explicitly specifying > $urlSuffix because it was not getting set in TestBaseApi.php. You need to set $urlPart in each test class to the URL file you're about to test. Then $urlSuffix is only necessary for i.e. URL parameters like "?id=foo" or such. > I assume that I'm missing a bunch of setup tasks because these tests > should not be able to pass as I first attempted to run them in > PHPUnit. Or maybe I should be running the tests all at once? No, every test should be runnable standalone and may never depend on data set/generated by previous tests. This has several reasons: - Tests just may be run alone by i.e. > phpunit --filter $nameoftestmethod . - You could activate process isolation in phpunit which means every test is run in an different php instance. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Brett D. <bs...@fr...> - 2011-04-15 02:11:21
|
Hello, Christian. I've got the relevant tests running for PostsAddTest.php but had another question: The remaining tests make HTTP requests to files that aren't part of the api (viz., edit.php, importNetscape.php, import.php, and bookmarks.php). Should I create new test files for these under tests/Api? It seems like they should go somewhere else. Maybe not a big issue, but worth asking. Rgrds, Brett On Thu, 14 Apr 2011 19:46:22 +0200, Christian Weiske <cw...@cw...> wrote: > Hello Brett, > > > >> I'm having trouble with a few items in PostsAddTest.php. I tried to >> run this test in PHPUnit and kept getting 404 responses from the Web >> server. Then I noticed line 36: >> >> protected $urlPart = 'api/posts/add'; >> >> I thought maybe this should be 'api/posts_add' and changed it. That >> eliminated the 404s, and so I thought it might be a mistake > I'll check that - we have URL rewriting rules that may affect that. > >>, but I was >> also unable to get the tests to run without explicitly specifying >> $urlSuffix because it was not getting set in TestBaseApi.php. > You need to set $urlPart in each test class to the URL file you're > about to test. Then $urlSuffix is only necessary for i.e. URL > parameters like "?id=foo" or such. > > >> I assume that I'm missing a bunch of setup tasks because these tests >> should not be able to pass as I first attempted to run them in >> PHPUnit. Or maybe I should be running the tests all at once? > No, every test should be runnable standalone and may never depend on > data set/generated by previous tests. > > This has several reasons: > - Tests just may be run alone by i.e. > > phpunit --filter $nameoftestmethod . > - You could activate process isolation in phpunit which means every > test is run in an different php instance. |
From: Christian W. <cw...@cw...> - 2011-04-15 04:31:39
Attachments:
signature.asc
|
Hello Brett, > I've got the relevant tests running for PostsAddTest.php but had > another question: > > The remaining tests make HTTP requests to files that aren't part of > the api (viz., edit.php, importNetscape.php, import.php, and > bookmarks.php). Should I create new test files for these under > tests/Api? It seems like they should go somewhere else. I'm writing some tests for the open search description and have some test for www/search.php - and put it under tests/www/searchTest.php I guess the will be reorganized a bit after we finished 0.98. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Christian W. <cw...@cw...> - 2011-04-29 04:59:46
Attachments:
signature.asc
|
Hello Brett, You have probably noticed that I'm reworking some of your tests to be smaller and more independent, and replacing some of the test code with more abstract methods. For example, you created a cookie yourself and relied on the knowledge how it is created internally. That is replaced with generic login method that does everything for you and gives you a http request full with all cookies needed. There were one or two bugs in that method which is why you probably didn't use it in the first place. Only some of this things to fix are left, then we can merge master into your branch and - when all is fine - merge your branch into master. I wrote some blog articles and had vacation last week which is why nothing happened on my side, but I'll probably get the last of your tests sorted out until next wednesday. Brett, I'd be happy if you could continue making the tests nicer and merge master - but merging is not trivial if you don't have a good graphical mergetool or really know what you do. I can do that without problems. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |
From: Brett D. <bs...@fr...> - 2011-05-02 22:12:27
|
Hello, Christian. I'll have a look at the unit tests this week. If I'm not able to clean them up any more, I can at least learn from the changes you've made. I am apprehensive about doing a merge myself since I am still relatively new to git. However, I will need to learn this at some point and will be installing msysgit soon. Rgrds, Brett On Fri, 29 Apr 2011 06:59:36 +0200, Christian Weiske <cw...@cw...> wrote: > Hello Brett, > > > You have probably noticed that I'm reworking some of your tests to be > smaller and more independent, and replacing some of the test code with > more abstract methods. > > For example, you created a cookie yourself and relied on the knowledge > how it is created internally. That is replaced with generic login > method that does everything for you and gives you a http request full > with all cookies needed. There were one or two bugs in that method > which is why you probably didn't use it in the first place. > > Only some of this things to fix are left, then we can merge master into > your branch and - when all is fine - merge your branch into master. > > I wrote some blog articles and had vacation last week which is why > nothing happened on my side, but I'll probably get the last of your > tests sorted out until next wednesday. > > Brett, I'd be happy if you could continue making the tests nicer and > merge master - but merging is not trivial if you don't have a good > graphical mergetool or really know what you do. I can do that without > problems. |
From: Christian W. <cw...@cw...> - 2011-05-03 05:16:02
Attachments:
signature.asc
|
Hello Brett, > I'll have a look at the unit tests this week. If I'm not able to clean > them up any more, I can at least learn from the changes you've made. I actually finished cleaning them up yesterday. What's left is moving most of them from PostsAddTest.php to files that match the URL that is tested. > I am apprehensive about doing a merge myself since I am still > relatively new to git. And I merged master yesterday, too - it wasn't as hard as I thought; only 6 files that had merge conflicts. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=- |