Skip to content
This repository has been archived by the owner on Oct 28, 2020. It is now read-only.

Test every weapon and vehicle call #12

Merged
merged 9 commits into from
Oct 24, 2017
Merged

Test every weapon and vehicle call #12

merged 9 commits into from
Oct 24, 2017

Conversation

ejdaduya
Copy link
Contributor

No description provided.

Copy link
Owner

@Girbons Girbons left a comment

Choose a reason for hiding this comment

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

Hi @ejdaduya I really appreciate your pull request but as i suggested here i'd like to have explicit tests.
Could you please follow the suggested code?
Thanks very much for your help.

@ejdaduya
Copy link
Contributor Author

Hi, as stated in line 86 & line 102, would you like to maybe add them to a log file or execute a specific task?

I've included a parameter as well in case you'd explicitly like to invoke the function.

I could make the explicit tests for you, but I thought you'd appreciate a dynamic function.

I'm pretty new here so please bear with me.

@Girbons
Copy link
Owner

Girbons commented Oct 23, 2017

@ejdaduya The submitted code is good but we have to make some changes, remove this
and the vehicles and weapons list

In the dynamic function I thought to do something like this:
Using the resolve weapon that you can find here

def weapon_call(weapon):
    bf = Battlefield('girbons', API_KEY, 'Pc')
    response = bf.progression_service.get_weapon(weapon=resolve_weapon(weapon))
    return response

Now you can use the dynamic function

def test_selbstlader_optical():
    response = weapon_call('selbstlader m1916 optical')
    assert response.status_code == 200

We can isolate tests for vehicles and weapons in 2 files

If you have another idea i'm glad to discuss it :)

@ejdaduya
Copy link
Contributor Author

Should we discard the test_resolver.py and create a test_weapon_resolver.py and test_vehicle_resolver.py then?

@Girbons
Copy link
Owner

Girbons commented Oct 23, 2017

@ejdaduya yes

Copy link
Owner

@Girbons Girbons left a comment

Choose a reason for hiding this comment

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

@ejdaduya nice work, change the last thing and add your name to the contributors file and i will be happy to merge your pull request

assert response.status_code == 200


def test_lawrence_of_arabia's_SMLE():
Copy link
Owner

Choose a reason for hiding this comment

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

change it in lower case test_lawrence_of_arabias_smle

@ejdaduya
Copy link
Contributor Author

Hi @Girbons, I've dropped some of the special characters for the function names. Kindly confirm if these meets your expectation.

@ejdaduya
Copy link
Contributor Author

Hi @Girbons, that's probably all from me for today.

@Girbons
Copy link
Owner

Girbons commented Oct 23, 2017

@ejdaduya I will take a look in this afternoon it seems good,
Could you please check with an api_key if the tests are passing?
I don't know why the CI is not using the API_KEY and now I can't check the issue on travis

@ejdaduya
Copy link
Contributor Author

Sure thing. I'll give you a feedback once I have some free time tonight.

@ejdaduya
Copy link
Contributor Author

Hi @Girbons, I've tried running the tests and the resolver function does not seem to be applicable for the test functions.

Here are my current issues:

  • resolve_weapon and resolve_vehicle returns the key value from the config file.
  • bf.progression_service.get_vehicle and bf.progression_service.get_weapon returns a dictionary object, not a requests response, thus assert response.status_code == 200 would fail as well.

I've tested this by running pytest tests/test_vehicle_resolver.py and pytest tests/test_weapon_resolver.py and simply providing my API_KEY from the utils file.

@Girbons
Copy link
Owner

Girbons commented Oct 24, 2017

@ejdaduya I missed a step, my bad

@Girbons Girbons merged commit fb922e5 into Girbons:master Oct 24, 2017
@Girbons
Copy link
Owner

Girbons commented Oct 24, 2017

Thank you very much @ejdaduya :)

@ejdaduya
Copy link
Contributor Author

Cheers!

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

Successfully merging this pull request may close these issues.

None yet

2 participants