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

Issue199 UI test #224

Merged
merged 13 commits into from
Oct 25, 2023
Merged

Issue199 UI test #224

merged 13 commits into from
Oct 25, 2023

Conversation

am2089
Copy link
Contributor

@am2089 am2089 commented Oct 24, 2023

What it Does

@am2089
Copy link
Contributor Author

am2089 commented Oct 24, 2023

So I did what you said
1)Made sure fork is up to date
2)Created a new branch
3) Made the copy and paste change into the ui file
4)made sure to only commit that one change
5)Made a new pr just to be sure and the other 8 commits are still there. You can see the latest commit which is for today, but the others aare still there. Its like they carried over.

@mikaelacaron
Copy link
Owner

2)Created a new branch

This step, when you created the new branch, make sure it was created from the dev branch, (which should only have the commits as the original repo (which is true for your fork, I looked at that). Do not create it based on the other branches!

That's most likely what happened, I'll look at it more in-depth later

Are you using Terminal to do git or some GUI?

@am2089
Copy link
Contributor Author

am2089 commented Oct 24, 2023

Terminal. What is the setting you use that indicates your on the dev. I usually see it on your live streams when your using the terminal. Right now I'm using default which is the original green one.

@mikaelacaron
Copy link
Owner

I use this, which shows the branch I'm on rather than needing to type git branch

https://ohmyz.sh

Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, it does technically add a vehicle, but an even better test would be to also confirm that the details from the vehicle that are added are also shown on the SettingsView

Because really what this is testing is that you an type into the text fields and press a button.

I'm going to manually fix your PR, but I'd suggest watching more videos about git and contributing to open source, because updating a feature branch is essential to know how to do. I think what you did was create a branch from your broken one that was never fixed, as opposed to making the branch from the dev branch. That's the only way the other commits "carried over"

Comment on lines 103 to 105
<<<<<<< HEAD
8288B89A2AE5C5C900AA21F9 /* AddVehicleViewUITest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AddVehicleViewUITest.swift; sourceTree = "<group>"; };
=======
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what a merge conflict looks like, this is something you have to fix, and not commit

func testAddVehicle() {
// Navigate to the AddVehicleView
app.buttons["Settings"].tap()
XCUIApplication().collectionViews/*@START_MENU_TOKEN@*/.buttons["Add Vehicle"]/*[[".cells.buttons[\"Add Vehicle\"]",".buttons[\"Add Vehicle\"]"],[[[-1,1],[-1,0]]],[0]]@END_MENU_TOKEN@*/.tap()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this /*@START_MENU_TOKEN@*/ stuff shouldn't be here. This is because you had Xcode auto fill something, but you never accepted it, look how it's "highlighted" you need to either double click on it, or click on it once and press enter to accept that as the code

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note you defined an app property in the class, yet here you're redefining using a new variable for XCUIApplication

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only works when there's one vehicle and that the Add Vehicle button is in the view immediately and you don't need to scroll to tap on it

@mikaelacaron
Copy link
Owner

See the final file changes, it's only a change to a single file, that's why I didn't want you to add a new file, when this is the only UI test in the project

@mikaelacaron mikaelacaron merged commit 4ea5791 into mikaelacaron:dev Oct 25, 2023
2 checks passed
@am2089
Copy link
Contributor Author

am2089 commented Oct 25, 2023

So since it was merged can I delete this branch?

@mikaelacaron
Copy link
Owner

Yes it can be deleted from your fork

@am2089
Copy link
Contributor Author

am2089 commented Oct 25, 2023

And for next time I understand what I did wrong (Even though you have been telling me to do it this whole time it just takes me a minute for me to realize) So I was not on the dev branch, and that branch was not updated that's why it was so messed up. Also, I know now to make the branch off the dev branch(As you also said lol). Once again thanks for being patient with me, much appreciated!

@am2089 am2089 deleted the Issue199-UITest branch October 25, 2023 05:08
@mikaelacaron
Copy link
Owner

It doesn't matter "being on the dev branch" but what matters is if your branch you are working on, which for you was Issue199-UITest, if you have updated it to match the dev branch (which didn't happen)

But yes when you redid this, what you needed to do was first be on the dev branch (have the dev branch checked out) and then create a new branch, what I think you did was make a new branched based on one of your other ones, that was still not updated

To update your branch (you merge dev into your feature branch)

This is all a learning opportunity! That's the entire point of this project, because this stuff is hard to recreate if you're only learning by yourself

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.

Write a UI Test to Add a Vehicle
2 participants