Count modules unit tests #1303

Closed
wants to merge 6 commits into
from

Projects

None yet

4 participants

@ghost
ghost commented Jun 22, 2012

Simple unit tests for CountModules added

@realityking
Member

Great stuff!

A couple of things:

  1. We don't accept failing tests. (I know we should but it's not possible in our current process)
  2. Could you rebase to get rid of the merge conflicts?

Also your patch revealed one of the interesting things that happened with the legacy package. JDocumentHTML actually depends on JModuleHelper but the later is in legacy and thus not available in unit tests. I propose to move it back since it's really an integral part of the templating system we have. What do other think?

@ghost
ghost commented Jun 22, 2012

By failing tests do you mean the first 3?

@elinw
Contributor
elinw commented Jun 24, 2012

@realityking I agree about moving it back. I think in the end when JContent comes in we might want to change the name/rebrand/reconceptualize or something and maybe move it, but there has to be a way to place bits of content on pages and until there is something new for that, modules in positions is something we have. I just don't think it's a useful framework for building web applications unless you have some concept of a location of a page that you can assign stuff to display in. That doesn't mean developers are in any way limited to how the CMS conceives of them.

@ghost
ghost commented Jun 25, 2012

The JModuleHelper error is coming from something outside my patch? IE what do I need to do to get this patch to pass the checks?

@elinw
Contributor
elinw commented Jun 26, 2012

@garyamort
I think you need to make one big pull request with both the tests (passing) and the fix. Then when the pulltester runs it the tests will pass.

@realityking
Member

I talked to Louis about this. Please add the test to the legacy test suite. That will work fine. You just have to move the file to a different folder for that.

@ghost
ghost commented Jul 6, 2012

Cool..thanks!

@realityking
Member

Any news on this?

@realityking
Member

@garyamort Could you fix this up as per my comment? Thanks.

@LouisLandry
Contributor

I'm closing this in favor of #1307. Please add these tests into that pull request and we'll get them merged in all at once. Thanks a bunch for this!

@LouisLandry LouisLandry closed this Oct 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment