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

[WIP] Adding XmlPeek for reading XML values. #641

Merged
merged 1 commit into from Jan 18, 2016

Conversation

Projects
None yet
3 participants
@patridge
Contributor

patridge commented Jan 16, 2016

Goal

Allow a build script to extract values from XML files.

Example

Task("PrintServerValue")
    .Does(() =>
{
    var file = File("test.xml");
    var serverValue = XmlPeek("/configuration/appSettings/add[@key = 'server']/@value");
    Information(serverValue);
});

Current State

I copied over XmlPoke code and its testing code as well. From there, I removed the writing logic, and set it to return a Value string.

Work Needed

I am terrible with XPath stuff, and I haven't been able to confirm what I've written will actually work. Unfortunately, I am working on a Mac using Xamarin Studio for these changes. I ran into a number of issues building and testing. I feel bad even putting this in a pull request, but I've been changing so many things trying to test it locally with NUnit, I wouldn't be sure I was even testing the same thing if I get it to work. Here are the issues I ran into that may or may not be easy to fix.

  • The current develop branch doesn't build at all, even before I made changes. Every instance of using Cake.Common.Tests.Properties; is a build error. I have these removed locally, but not committed.
  • Running xUnit tests on Xamarin Studio is problematic. I haven't figured out how to get it to happen at all.
  • Running ./build.sh from the command line built it fine, but failed on every non-skipped unit tests with the following error.

System.Resources.MissingManifestResourceException : Could not find any resources appropriate for the specified culture or the neutral culture. Make sure "Cake.Common.Tests.Resources.resources" was correctly embedded or linked into assembly "Cake.Common.Tests" at compile time, or that all the satellite assemblies required are loadable and fully signed.

I don't know if these problems are all specific to my machine or to my environment in general (OS X, XS, etc.). The first item above could even be a preexisting issue with current develop branch.

@patridge

This comment has been minimized.

Contributor

patridge commented Jan 16, 2016

It seems I've also got some StyleCop work to do. I'll do my best to fix things blindly from XS.

@patriksvensson

This comment has been minimized.

Member

patriksvensson commented Jan 16, 2016

@patridge Thanks for your contribution 👍

First of all, let me say that I'm sorry for the bad Xamarin Studio experience you got when building this. I don't use Xamarin Studio to work on this project so it's kind of not supported at the moment, but I would absolutely want this to work. StyleCop is also an issue that we need to solve on *nix, but we haven't got around to do any work on it yet. We build and run tests via our CI server though (Linux and OS X), so it should build and run on Mono without problem.

Tell me when you feel happy with the PR and I'll review it.

Thanks!

@patriksvensson

This comment has been minimized.

Member

patriksvensson commented Jan 16, 2016

@patridge I've created an issue for this: #642

@patridge

This comment has been minimized.

Contributor

patridge commented Jan 16, 2016

@patriksvensson No need to apologize. Sorry in advance for how noisy this PR might get as I push through the CI errors. I'll mention you again (after this) when I feel like I've done as much as I can.

While it is only a partial fix, Xamarin Studio can store style preferences in the solution file. While it isn't enforced anywhere, it could prevent a few potential issues (especially tabs/spaces stuff).

@patridge

This comment has been minimized.

Contributor

patridge commented Jan 16, 2016

@patriksvensson I guess I might be bugging you before I'm done. The current issue I'm seeing is what I was seeing before I even started adding code (first bullet in the original PR comment above regarding Properties not existing for using Cake.Common.Tests.Properties;).

I have the changes committed and will push them up, but I'm not sure how develop was building properly before I touched things.

@patriksvensson

This comment has been minimized.

Member

patriksvensson commented Jan 16, 2016

@patridge Ah, it seems like Xamarin Studio is messing with src/Cake.Common.Tests/Properties/Resources.Designer.cs and rewriting paths to things.

Maybe you should add the XML directly to the fixture and revert the resources until we know how we can fix this.

@patriksvensson patriksvensson referenced this pull request Jan 16, 2016

Closed

XmlPeek alias #598

@patridge

This comment has been minimized.

Contributor

patridge commented Jan 16, 2016

@patriksvensson Will do for now. I definitely don't have a lot of experience with embedded resources, so I won't be much help resolving the XS issue with things.

@patriksvensson

This comment has been minimized.

Member

patriksvensson commented Jan 16, 2016

@patridge Perfect! Ping me if I can help out with anything.

@patridge

This comment has been minimized.

Contributor

patridge commented Jan 16, 2016

I figured out what Mono did to Resource.designer.cs and manually corrected it with a text editor. Now the new unit tests are actually failing in my ./build.sh calls locally. Clearly, my blind coding isn't quite perfect, but at least it is something I can work with. 😃

@patridge

This comment has been minimized.

Contributor

patridge commented Jan 16, 2016

I had a major XML oops in there with multiple root elements so I could test a text node. Then, I had to relearn some XPath for how to get a node's text. But that is all worked out now.

@patriksvensson All seems to be well now, and tests are passing. I'm sure there are other project-specific things to address now (e.g., docs and such), so let me know if there are other things needed to make this acceptable.

/// <param name="source">The source xml to transform.</param>
/// <param name="xpath">The xpath of the nodes to set.</param>
/// <param name="settings">Additional settings to tweak Xml Poke behavior.</param>
private static string XmlPeek(XmlReader source, string xpath, XmlPokeSettings settings)

This comment has been minimized.

@patriksvensson

patriksvensson Jan 17, 2016

Member

Would prefer if we added a XmlPeekSettings here.

This comment has been minimized.

@patridge

patridge Jan 17, 2016

Contributor

Addressed in a new commit.

var node = document.SelectSingleNode(xpath, namespaceManager);
if (node == null)
{
const string format = "Failed to find node matching the XPath '{0}'";

This comment has been minimized.

@patriksvensson

patriksvensson Jan 17, 2016

Member

Since we're only querying for a value, wouldn't it be better to log a warning (Failed to find) and return null? This case the consumer don't have to wrap this in try/catch clause.

This comment has been minimized.

@patridge

patridge Jan 17, 2016

Contributor

Good call. I definitely prefer that approach.

I see that I'll have access to an ICakeLog via context.Log in the extension methods, but I don't suppose you can point me to a good example to reference/duplicate for implementing this system?

This comment has been minimized.

@patridge

patridge Jan 17, 2016

Contributor

I found the various logging extension methods and put something in place in this commit.

namespace Cake.Common.Tests.Unit.XML
{
public sealed class XmlPeekTests

This comment has been minimized.

@patriksvensson

patriksvensson Jan 17, 2016

Member

This should be called XmlPeekAliasesTests.

Would prefer it you use the same naming conventions as other tests:
XmlPeekAliasesTests -> TheXmlPeekMethod -> Should_Throw_If_FilePath_Is_Null

This comment has been minimized.

@patridge

patridge Jan 17, 2016

Contributor

Can do. Since I'd only been basing things on XmlPokeTests, I didn't realize it, and my derived code, diverged from the preferred conventions.

I made changes trying to match what you describe in this commit.

namespace Cake.Common.Tests.Fixtures
{
internal sealed class XmlPeekFixture

This comment has been minimized.

@patriksvensson

patriksvensson Jan 17, 2016

Member

Should be called XmlPeekAliasesFixture.

This comment has been minimized.

@patridge

patridge Jan 17, 2016

Contributor

Addressed in this commit.

@patriksvensson

This comment has been minimized.

Member

patriksvensson commented Jan 17, 2016

@patridge Great work! I left some comments for you.

@patridge

This comment has been minimized.

Contributor

patridge commented Jan 17, 2016

@patriksvensson I did my best to address the comments.

@patriksvensson

This comment has been minimized.

Member

patriksvensson commented Jan 17, 2016

@patridge Found a xml doc comment that wasn't updated, but other than that it looks good to me!

After you've fixed the comment, could you rebase against develop and squash to one commit so I can merge this. Thanks! 👍

@gep13

This comment has been minimized.

Member

gep13 commented Jan 17, 2016

@patridge this is great work! Thanks for your contribution! 👍

Adam Patridge Adam Patridge
@patridge

This comment has been minimized.

Contributor

patridge commented Jan 18, 2016

I've only done a couple interactive rebases before, and only one successfully ending where I wanted things. I think things worked out again. If not, I saved things in a local branch if I need to try again.

@patriksvensson I fixed that doc comment before I did the rebase. Let me know if anything isn't ready to go.

@gep13

This comment has been minimized.

Member

gep13 commented Jan 18, 2016

@patridge interactive rebases can be problematic, and a little scary, when you start to do them, but after a while, they become second nature. I now routinely do interactive rebases as part of my development workflow. It is a very powerful tool. Seems like things have worked well here in this PR 👍

@patriksvensson

This comment has been minimized.

Member

patriksvensson commented Jan 18, 2016

@patridge Great! Will take a look at it as soon as I can. 👍

@patriksvensson patriksvensson merged commit e1d7684 into cake-build:develop Jan 18, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment