search

corcel / acf

Advanced Custom Fields (ACF) plugin for Corcel
128 stars 100 forks source link

Changes to allow basic field to fetch fields with empty (but not null fields). #11

Closed cjke closed 7 years ago

cjke commented 7 years ago

This is in relation to #4

Note that the tests use assertSame instead of assertEqual because with assertEqual returns true for assertEqual("", null), which is not what we want.

Results as per the contributing guide:

ubuntu@wordpress /v/w/s/acf> ./vendor/phpunit/phpunit/phpunit
PHPUnit 4.2.2 by Sebastian Bergmann.

Configuration read from /var/www/sites/acf/phpunit.xml

................................................

Time: 510 ms, Memory: 8.00MB

OK (48 tests, 92 assertions)

Out of interest, you can run the new tests without the changes to BasicField and see how pulling out a "" wasn't possible.

Also note, I needed to add a new Page in WP with all the empty fields (the new sql dump includes that):

image

This was added to the Custom Field manually (also in the sql dump)

image

Btw: That first PR failed because I didn't zip up the correct sql dump.

cjke commented 7 years ago

@jgrossi Not sure why Travis is failing. It's marking the BasicField constructor as missing the Post argument - but the dev branch doesn't take a Post argument in the constructor any more (only in master).

cjke commented 7 years ago

Can I confirm that I am merging against the right branch according to your contribution guidelines?

jgrossi commented 7 years ago

hi @cjke there are 7 test failing... did you confirm all the tests are passing locally?

jgrossi commented 7 years ago

the branch is correct. I think it's not necessary testing empty fields for all fields in that page, once all of them are Text field and you are testing the same get() method.

cjke commented 7 years ago

Hi. Yes all tests are passing locally.

cjke commented 7 years ago

And yes, agree about the overlap of tests but:

  1. without code coverage in place thats impossible to tell (so if someone revisits the tests in a year, its not clear why some are swapped and why some aren't)
  2. tests should be "unaware" of the underlying workings of a module - we should be testing the public methods, what's exposed as part of the public api of said module.

At the end of the day, your library, so your call - but wanted to explain the reasoning.

In terms of the tests failing - as mentioned above, it seems that the test cases are running about the library when it was expecting the BasicField constructor to accept an argument (which isn't the case in dev anymore).

image