Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added display current design settings on particular store #535

Merged
merged 1 commit into from May 2, 2015
Merged

Added display current design settings on particular store #535

merged 1 commit into from May 2, 2015

Conversation

LukeOl
Copy link
Contributor

@LukeOl LukeOl commented Apr 27, 2015

Hi @tkn98

OK, I am creating new PR, as you asked, this time against develop branch.
I can see now, there is 1 commit and 2 files changed, instead of 22 commits to merge :)
I have also changed my repo username, so it will fit now to github account.
Previous PR was: #531 and #534

I am pasting my original message to you guys:
First of all, great work guys with magerun, I can't imagine to work with Magento without Magerun :)

I have proposition of new command to Magerun pack.
I have added command to magerun, which shows current design settings on particular store view.

When developer works on Magento with one store view, he/she simply goes to configuration to obtain design settings.
If store has more than one store view, it is annoying to obtain design settings on all store views.
Command dev:theme:info will quickly show design settings to all Magento store views.

I hope this will be useful for people. I can assure you, our frontend team is pleased with this command :)

Greetz, LukeOl

@tkn98
Copy link
Collaborator

tkn98 commented Apr 27, 2015

Great to see that it worked now with the branch of the PR!

Now something is borken with the composer install on travis. Most likely not your fault. I created a new issue for that problem alone.

@tkn98 tkn98 mentioned this pull request Apr 27, 2015
@tkn98
Copy link
Collaborator

tkn98 commented Apr 27, 2015

@LukeOl I could further review the problem with the failed builds. Part of the problem is low code-coverage. Do you have some experience and/or interest in writing unit-tests with Phpunit?

@LukeOl
Copy link
Contributor Author

LukeOl commented Apr 27, 2015

Hi, Unfortunately about unit test I am at baby level so at this moment I can't help you with utests :(

One thing is weird to me, PR #531 has passed travis build and current one does not. How?
I am guessing that in #531 I have added total different class as a new command (CurrentCommand), and in current one I modified exists class (InfoCommand).
But I haven't seen any test file in /test/N98 related to InfoCommand class...

@pocallaghan
Copy link
Contributor

By contributing new code which isn't covered by unit tests, you lower the total code coverage across the project. The reason your previous pull request didn't cause the build failure is probably because other pull requests without unit tests have already been merged into develop, thus the overall code coverage is lower than it is on master. Therefore your code is the proverbial "straw that broke the camels back". At least that's how I understand it.

I'm conscious that I myself have made pull requests without adding/amending unit tests, but since I'm inexperienced when it comes to writing tests I find it difficult working out how to compose meaningful tests. I feel that to a certain degree the way code is organised within the project doesn't always lend itself to unit testing. Most of the logic is contained within the commands themselves, rather than the command being a thin 'controller' layer which proxies information to a 'model' layer. This tends to make the tests feel more like functional / integration tests rather than 'true' unit tests. It's difficult for example to mock objects, since they are instantiated within the command rather than injected as dependencies.

Don't get me wrong I'm not trying to be critical, it's an inherent problem with the Magento eco-structure to a large degree. I'm just highlighting that IMO it's difficult moving from "Hello World" tutorials on unit testing to providing meaningful tests to a project like this. Taking this pull request as an example, what exactly could you test? Without loading test fixtures into a Magento database (which I don't think there's a framework for?) to assert that the given output matches the loaded fixtures, I'm not sure how you'd test it.

@tkn98
Copy link
Collaborator

tkn98 commented Apr 27, 2015

@LukeOl Don't fear, that's why I'm just asking.

One thing is weird to me, PR #531 has passed travis build and current one does not. How?

I suspect this is because we're currently on the edge of the percentage value where we say the build is still okay or has failed in terms of code-coverage. This is also the reason why I asked to learn whether or not you have interested in improving this coverage situation with new unit-tests or we need just to take a look here on project level how to cover that. BTW it's not only you, other PRs have lowered the coverage metric, too.

You suggestion is a good one, I'll take a look if InfoCommand is a good candidate to improve this.

@pocallaghan: Like writing code, writing tests is a process. You can't expect your first tests to be perfect but contributing code with tests in FLOSS projects can give you good feedback and review. Tests are welcome, if you feel uncertain, just start with a small one and ask for feedback. We all started some time and practice makes perfect.

cmuench added a commit that referenced this pull request May 2, 2015
…lop_to_develop

Added display current design settings on particular store
@cmuench cmuench merged commit 90a5a30 into netz98:develop May 2, 2015
@cmuench
Copy link
Member

cmuench commented May 2, 2015

@LukeOl Thanks for contribution. I will add some little integration tests here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants